Skip to content

Fix succeeded_power calculation in PV power distribution #1217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 15, 2025
Merged

Fix succeeded_power calculation in PV power distribution #1217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d877386
Fix `succeeded_power` calculation in PV power distribution
shsms May 15, 2025
02fbd21
Update release notes
shsms May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@

## Bug Fixes

- Components used to be just forgotten by the power manager when all proposals are withdrawn, leaving them at their last set power values. This has been fixed by getting the power manager to set the components to their default powers, based on the component category (according to the table below), as the last step.
- The power manager used to just forgot components when all proposals to them are withdrawn, leaving them at their last set power values. This has been fixed by getting the power manager to set the components to their default powers, based on the component category (according to the table below), as the last step.


| component category | default power |
Expand All @@ -58,4 +58,6 @@

- PV Pool instances can now be created in sites without any PV. This allows for writing generic code that works for all locations, that depends on the PV power formula, for example.

- `Success/PartialFailure` results from `PVPool.power_distribution_results` now report correct `succeeded_power` values.

- The `find_first_descendant_component` method in the component graph was allowing non-root components to be used as the root component during traversal. This was leading to confusing behaviour when the root component couldn't be identified deterministically. For example, if the root category was specified as a meter, it could start traversing from a different meter each time. It is no-longer possible to specify a root category anymore and it always traverses from the `GRID` component.
9 changes: 3 additions & 6 deletions ...grid/_power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import logging
from datetime import timedelta

from frequenz.channels import Broadcast, LatestValueCache, Sender
from frequenz.channels import LatestValueCache, Sender
from frequenz.client.microgrid import (
ApiClientError,
ComponentCategory,
Expand Down Expand Up @@ -67,9 +67,6 @@ def __init__(
self._component_data_caches: dict[
ComponentId, LatestValueCache[InverterData]
] = {}
self._target_power = Power.zero()
self._target_power_channel = Broadcast[Request](name="target_power")
self._target_power_tx = self._target_power_channel.new_sender()
self._task: asyncio.Task[None] | None = None

@override
Expand Down Expand Up @@ -241,7 +238,7 @@ async def _set_api_power( # pylint: disable=too-many-locals
failed_components=failed_components,
succeeded_components=succeeded_components,
failed_power=failed_power,
succeeded_power=self._target_power - failed_power,
succeeded_power=request.power - failed_power - remaining_power,
Copy link

Copilot AI May 15, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding a brief comment or updating the docstring here to clarify why the succeeded_power is calculated as request.power minus both the failed_power and remaining_power.

Copilot uses AI. Check for mistakes.
excess_power=remaining_power,
request=request,
)
Expand All @@ -250,7 +247,7 @@ async def _set_api_power( # pylint: disable=too-many-locals
await self._results_sender.send(
Success(
succeeded_components=succeeded_components,
succeeded_power=self._target_power,
succeeded_power=request.power - remaining_power,
Copy link

Copilot AI May 15, 2025

Choose a reason for hiding this comment

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

[nitpick] A comment explaining why the succeeded_power calculation in this branch excludes failed_power (compared to the previous branch) would improve maintainability and clarity.

Copilot uses AI. Check for mistakes.
excess_power=remaining_power,
request=request,
)
Expand Down
104 changes: 100 additions & 4 deletions tests/timeseries/_pv_pool/test_pv_pool_control_methods.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ async def test_setting_power( # pylint: disable=too-many-statements
bounds_rx,
lambda x: x.bounds is not None and x.bounds.lower.as_watts() == -100000.0,
)
dist_results_rx = pv_pool.power_distribution_results.new_receiver()

self._assert_report(latest_report, power=None, lower=-100000.0, upper=0.0)
await pv_pool.propose_power(Power.from_watts(-80000.0))
await self._recv_reports_until(
Expand All @@ -172,6 +174,18 @@ async def test_setting_power( # pylint: disable=too-many-statements
mocker.call(inv_ids[2], -25000.0),
mocker.call(inv_ids[3], -25000.0),
]
dist_results = await dist_results_rx.receive()
assert isinstance(
dist_results, _power_distributing.Success
), f"Expected a success, got {dist_results}"
assert dist_results.succeeded_power == Power.from_watts(-80000.0)
assert dist_results.excess_power == Power.zero()
assert dist_results.succeeded_components == {
ComponentId(8),
ComponentId(18),
ComponentId(28),
ComponentId(38),
}

set_power.reset_mock()
await pv_pool.propose_power(Power.from_watts(-4000.0))
Expand All @@ -194,6 +208,18 @@ async def test_setting_power( # pylint: disable=too-many-statements
mocker.call(inv_ids[2], -1000.0),
mocker.call(inv_ids[3], -1000.0),
]
dist_results = await dist_results_rx.receive()
assert isinstance(
dist_results, _power_distributing.Success
), f"Expected a success, got {dist_results}"
assert dist_results.succeeded_power == Power.from_watts(-4000.0)
assert dist_results.excess_power == Power.zero()
assert dist_results.succeeded_components == {
ComponentId(8),
ComponentId(18),
ComponentId(28),
ComponentId(38),
}

# After failing 1 inverter, bounds should go down and power shouldn't be
# distributed to that inverter.
Expand All @@ -205,6 +231,17 @@ async def test_setting_power( # pylint: disable=too-many-statements
self._assert_report(
await bounds_rx.receive(), power=-4000.0, lower=-80000.0, upper=0.0
)
dist_results = await dist_results_rx.receive()
assert isinstance(
dist_results, _power_distributing.Success
), f"Expected a success, got {dist_results}"
assert dist_results.succeeded_power == Power.from_watts(-4000.0)
assert dist_results.excess_power == Power.zero()
assert dist_results.succeeded_components == {
ComponentId(8),
ComponentId(28),
ComponentId(38),
}

set_power.reset_mock()
await pv_pool.propose_power(Power.from_watts(-70000.0))
Expand All @@ -227,6 +264,17 @@ async def test_setting_power( # pylint: disable=too-many-statements
mocker.call(inv_ids[2], -30000.0),
mocker.call(inv_ids[3], -30000.0),
]
dist_results = await dist_results_rx.receive()
assert isinstance(
dist_results, _power_distributing.Success
), f"Expected a success, got {dist_results}"
assert dist_results.succeeded_power == Power.from_watts(-70000.0)
assert dist_results.excess_power == Power.zero()
assert dist_results.succeeded_components == {
ComponentId(8),
ComponentId(28),
ComponentId(38),
}

# After the failed inverter recovers, bounds should go back up and power
# should be distributed to all inverters
Expand All @@ -238,17 +286,29 @@ async def test_setting_power( # pylint: disable=too-many-statements
self._assert_report(
await bounds_rx.receive(), power=-70000.0, lower=-100000.0, upper=0.0
)
dist_results = await dist_results_rx.receive()
assert isinstance(
dist_results, _power_distributing.Success
), f"Expected a success, got {dist_results}"
assert dist_results.succeeded_power == Power.from_watts(-70000.0)
assert dist_results.excess_power == Power.zero()
assert dist_results.succeeded_components == {
ComponentId(8),
ComponentId(18),
ComponentId(28),
ComponentId(38),
}

set_power.reset_mock()
await pv_pool.propose_power(Power.from_watts(-90000.0))
await pv_pool.propose_power(Power.from_watts(-200000.0))
await self._recv_reports_until(
bounds_rx,
lambda x: x.target_power is not None
and x.target_power.as_watts() == -90000.0,
and x.target_power.as_watts() == -100000.0,
)

self._assert_report(
await bounds_rx.receive(), power=-90000.0, lower=-100000.0, upper=0.0
await bounds_rx.receive(), power=-100000.0, lower=-100000.0, upper=0.0
)
await asyncio.sleep(0.0)

Expand All @@ -258,8 +318,20 @@ async def test_setting_power( # pylint: disable=too-many-statements
mocker.call(inv_ids[0], -10000.0),
mocker.call(inv_ids[1], -20000.0),
mocker.call(inv_ids[2], -30000.0),
mocker.call(inv_ids[3], -30000.0),
mocker.call(inv_ids[3], -40000.0),
]
dist_results = await dist_results_rx.receive()
assert isinstance(
dist_results, _power_distributing.Success
), f"Expected a success, got {dist_results}"
assert dist_results.succeeded_power == Power.from_watts(-100000.0)
assert dist_results.excess_power == Power.zero()
assert dist_results.succeeded_components == {
ComponentId(8),
ComponentId(18),
ComponentId(28),
ComponentId(38),
}

# Setting 0 power should set all inverters to 0
set_power.reset_mock()
Expand All @@ -281,6 +353,18 @@ async def test_setting_power( # pylint: disable=too-many-statements
mocker.call(inv_ids[2], 0.0),
mocker.call(inv_ids[3], 0.0),
]
dist_results = await dist_results_rx.receive()
assert isinstance(
dist_results, _power_distributing.Success
), f"Expected a success, got {dist_results}"
assert dist_results.succeeded_power == Power.zero()
assert dist_results.excess_power == Power.zero()
assert dist_results.succeeded_components == {
ComponentId(8),
ComponentId(18),
ComponentId(28),
ComponentId(38),
}

# Resetting the power should lead to default (full) power getting set for all
# inverters.
Expand All @@ -301,3 +385,15 @@ async def test_setting_power( # pylint: disable=too-many-statements
mocker.call(inv_ids[2], -30_000.0),
mocker.call(inv_ids[3], -40_000.0),
]
dist_results = await dist_results_rx.receive()
assert isinstance(
dist_results, _power_distributing.Success
), f"Expected a success, got {dist_results}"
assert dist_results.succeeded_power == Power.from_watts(-100000.0)
assert dist_results.excess_power == Power.zero()
assert dist_results.succeeded_components == {
ComponentId(8),
ComponentId(18),
ComponentId(28),
ComponentId(38),
}
Loading