Skip to content

Conversation

@shsms
Copy link
Contributor

@shsms shsms commented May 5, 2025

The PV manager and the EV Charger manager of the PowerDistributor are
already using Power and not float.

@Copilot Copilot AI review requested due to automatic review settings May 5, 2025 16:14
@shsms shsms requested a review from a team as a code owner May 5, 2025 16:14
@shsms shsms requested review from Marenz and removed request for a team May 5, 2025 16:14
@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline part:microgrid Affects the interactions with the microgrid labels May 5, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request refactors the PowerDistributor’s battery management by replacing float‐based power values with the new Power type. Key changes include updates to test assertions involving PowerBounds, modifications in the metric calculations and distribution algorithm to use Power objects, and corresponding adjustments in the battery manager’s API interactions.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/microgrid/power_distributing/test_power_distributing.py Updated tests to construct PowerBounds using Power.from_watts for all power parameters.
src/frequenz/sdk/timeseries/battery_pool/_metric_calculator.py Revised arithmetic calculations to work with Power objects rather than raw floats.
src/frequenz/sdk/microgrid/_power_distributing/result.py Changed type annotations from float to Power in PowerBounds.
src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py Updated distribution algorithm to use Power objects and their arithmetic methods.
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py Adjusted battery manager logic to operate with Power values and consistently convert to watts when interfacing with external APIs.
Comments suppressed due to low confidence (3)

tests/microgrid/power_distributing/test_power_distributing.py:969

  • Confirm that Power.from_watts(math.nan) returns the expected NaN behavior; consider adding explicit test cases to validate how NaN values are handled.
Power.from_watts(math.nan),

src/frequenz/sdk/timeseries/battery_pool/_metric_calculator.py:605

  • [nitpick] Consider leveraging operator overloading in the Power type to directly perform arithmetic operations, which would reduce the need for manual conversion using Power.from_watts on aggregated float sums.
sum((bound.inclusion_lower for bound in inverter_bounds), start=Power.zero()),

src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py:641

  • Verify that the set_power API expects watt values; using power.as_watts() here should align with the API contract and remain consistent throughout the codebase.
api.set_power(inverter_id, power.as_watts())

@shsms shsms added the cmd:skip-release-notes It is not necessary to update release notes for this PR label May 5, 2025
@shsms
Copy link
Contributor Author

shsms commented May 5, 2025

@llucax I hope this won't be too disruptive for your microgrid API upgrade work. Otherwise, we can postpone this.

The PV manager and the EV Charger manager of the PowerDistributor are
already using `Power` and not `float`.

Signed-off-by: Sahas Subramanian <[email protected]>
@shsms shsms force-pushed the power-dist-bat-power branch from 2af4078 to 41d4543 Compare May 5, 2025 16:21
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it should be fine. I can maybe try to rebase/merge the PR to the current head and then to this PR to see if it is a mess or not.

I also noticed component ID are still int, I thought I already merged the change to ID types, but maybe I only merged that in the client and never did the SDK PR to update to the new client... 😱

@llucax
Copy link
Contributor

llucax commented May 6, 2025

BTW, for the records LGTM, not approving only for checking the status of the client update PR.

@shsms shsms enabled auto-merge May 6, 2025 13:25
@shsms shsms requested a review from llucax May 6, 2025 13:26
@shsms
Copy link
Contributor Author

shsms commented May 9, 2025

Any news @llucax?

@shsms shsms added this pull request to the merge queue May 12, 2025
@github-project-automation github-project-automation bot moved this from To do to Review approved in Python SDK Roadmap May 12, 2025
@shsms shsms removed this pull request from the merge queue due to a manual request May 12, 2025
@shsms
Copy link
Contributor Author

shsms commented May 12, 2025

I also noticed component ID are still int, I thought I already merged the change to ID types, but maybe I only merged that in the client and never did the SDK PR to update to the new client... 😱

You did make a PR, but wasn't merged because of test failures in python 3.12: #1182

@llucax
Copy link
Contributor

llucax commented May 12, 2025

Oh damn, my backlog is getting bigger and bigger... 🙈

@shsms shsms added this pull request to the merge queue May 12, 2025
Merged via the queue into frequenz-floss:v1.x.x with commit de4e062 May 12, 2025
5 checks passed
@shsms shsms deleted the power-dist-bat-power branch May 12, 2025 09:30
@github-project-automation github-project-automation bot moved this from Review approved to Done in Python SDK Roadmap May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR part:data-pipeline Affects the data pipeline part:microgrid Affects the interactions with the microgrid part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

Development

Successfully merging this pull request may close these issues.

3 participants