diff --git a/src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py b/src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py index b4e390e1d..92dcc1e72 100644 --- a/src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py +++ b/src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py @@ -609,8 +609,12 @@ def _distribute_multi_inverter_pairs( else: remaining_power = power.power - # Inverters are sorted by largest excl bound first - for inverter_id in inverter_ids: + # Sort inverters to have the largest exclusion bounds first + sorted_inverter_ids = sorted( + inverter_ids, key=lambda inv_id: excl_bounds[inv_id], reverse=True + ) + + for inverter_id in sorted_inverter_ids: if ( not is_close_to_zero(remaining_power) and excl_bounds[inverter_id] <= remaining_power diff --git a/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py b/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py index 6c8fc8751..58b037f93 100644 --- a/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py +++ b/src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py @@ -144,8 +144,9 @@ def _validate_component_ids( for bucket in self._component_buckets: if any(component_id in bucket for component_id in component_ids): + comp_ids = ", ".join(map(str, sorted(component_ids))) raise NotImplementedError( - f"PowerManagingActor: component IDs {component_ids} are already" + f"PowerManagingActor: component IDs {comp_ids} are already" " part of another bucket. Overlapping buckets are not" " yet supported." ) diff --git a/src/frequenz/sdk/microgrid/component_graph.py b/src/frequenz/sdk/microgrid/component_graph.py index f5cdc081e..5c01304bf 100644 --- a/src/frequenz/sdk/microgrid/component_graph.py +++ b/src/frequenz/sdk/microgrid/component_graph.py @@ -915,14 +915,14 @@ def _validate_graph(self) -> None: # This check doesn't seem to have much sense, it only search for nodes without # data associated with them. We leave it here for now, but we should consider # removing it in the future. - undefined: list[int] = [ + if undefined := [ node[0] for node in self._graph.nodes(data=True) if len(node[1]) == 0 - ] - - if len(undefined) > 0: + ]: + undefined_str = ", ".join(map(str, sorted(undefined))) raise InvalidGraphError( - f"Missing definition for graph components: {undefined}" + f"Missing definition for graph components: {undefined_str}" ) + # should be true as a consequence of checks above if sum(1 for _ in self.components()) <= 0: raise InvalidGraphError("Graph must have a least one component!") diff --git a/tests/actor/_power_managing/test_matryoshka.py b/tests/actor/_power_managing/test_matryoshka.py index 03aabff36..589e7b139 100644 --- a/tests/actor/_power_managing/test_matryoshka.py +++ b/tests/actor/_power_managing/test_matryoshka.py @@ -435,8 +435,8 @@ async def test_matryoshka_drop_old_proposals() -> None: with pytest.raises( NotImplementedError, match=re.escape( - "PowerManagingActor: component IDs frozenset({8, 5}) are already " - + "part of another bucket. Overlapping buckets are not yet supported." + "PowerManagingActor: component IDs 5, 8 are already part of " + "another bucket. Overlapping buckets are not yet supported." ), ): tester.tgt_power( @@ -506,8 +506,8 @@ def ensure_overlapping_bucket_request_fails() -> None: with pytest.raises( NotImplementedError, match=re.escape( - "PowerManagingActor: component IDs frozenset({8, 5}) are already " - + "part of another bucket. Overlapping buckets are not yet supported." + "PowerManagingActor: component IDs 5, 8 are already part of " + "another bucket. Overlapping buckets are not yet supported." ), ): tester.tgt_power( diff --git a/tests/actor/power_distributing/test_battery_distribution_algorithm.py b/tests/actor/power_distributing/test_battery_distribution_algorithm.py index 0ecf37a81..4397fc050 100644 --- a/tests/actor/power_distributing/test_battery_distribution_algorithm.py +++ b/tests/actor/power_distributing/test_battery_distribution_algorithm.py @@ -4,6 +4,7 @@ # pylint: disable=too-many-lines """Tests for distribution algorithm.""" import math +from collections.abc import Sequence, Set from dataclasses import dataclass from datetime import datetime, timezone @@ -1023,6 +1024,20 @@ def assert_result(result: DistributionResult, expected: DistributionResult) -> N assert result.distribution == approx(expected.distribution, abs=0.01) assert result.remaining_power == approx(expected.remaining_power, abs=0.01) + @staticmethod + def assert_any_result( + result: DistributionResult, + expected_distribution_ids: Set[int], + expected_distribution_powers: Sequence[float], + expected_remaining_power: float, + ) -> None: + """Assert the result is as expected, disregarding which power goes to which component.""" + assert result.remaining_power == expected_remaining_power + assert {*result.distribution.keys()} == expected_distribution_ids + assert list(sorted(result.distribution.values())) == list( + sorted(expected_distribution_powers) + ) + def test_scenario_1(self) -> None: """Test scenario 1. @@ -1349,19 +1364,27 @@ def test_scenario_4(self) -> None: algorithm = BatteryDistributionAlgorithm() - self.assert_result( + # The assignment of power to each inverter can be swapped, as they both have the + # same bounds, none will be preferred + self.assert_any_result( algorithm.distribute_power(-300, components), - DistributionResult({2: -300, 3: 0}, remaining_power=0.0), + expected_distribution_ids={2, 3}, + expected_distribution_powers=[-300, 0], + expected_remaining_power=0, ) - self.assert_result( + self.assert_any_result( algorithm.distribute_power(300, components), - DistributionResult({2: 300, 3: 0}, remaining_power=0.0), + expected_distribution_ids={2, 3}, + expected_distribution_powers=[300, 0], + expected_remaining_power=0, ) - self.assert_result( + self.assert_any_result( algorithm.distribute_power(-1800, components), - DistributionResult({2: -1000, 3: -500}, remaining_power=-300.0), + expected_distribution_ids={2, 3}, + expected_distribution_powers=[-1000, -500], + expected_remaining_power=-300, ) def test_scenario_5(self) -> None: @@ -1431,33 +1454,37 @@ def test_scenario_5(self) -> None: algorithm = BatteryDistributionAlgorithm() - self.assert_result( + self.assert_any_result( algorithm.distribute_power(-300, components), - DistributionResult({10: -200, 11: 0, 20: -100, 21: 0}, remaining_power=0.0), + expected_distribution_ids={10, 11, 20, 21}, + expected_distribution_powers=[-200, 0, -100, 0], + expected_remaining_power=0.0, ) - self.assert_result( + self.assert_any_result( algorithm.distribute_power(300, components), - DistributionResult({10: 200, 11: 0, 20: 100, 21: 0}, remaining_power=0.0), + expected_distribution_ids={10, 11, 20, 21}, + expected_distribution_powers=[200, 0, 100, 0], + expected_remaining_power=0.0, ) - self.assert_result( + self.assert_any_result( algorithm.distribute_power(-1800, components), - DistributionResult( - {10: -300, 11: 0, 20: -1000, 21: -500}, remaining_power=0.0 - ), + expected_distribution_ids={10, 11, 20, 21}, + expected_distribution_powers=[-300, 0, -1000, -500], + expected_remaining_power=0.0, ) - self.assert_result( + self.assert_any_result( algorithm.distribute_power(3000, components), - DistributionResult( - {10: 1000, 11: 500, 20: 1000, 21: 500}, remaining_power=0.0 - ), + expected_distribution_ids={10, 11, 20, 21}, + expected_distribution_powers=[1000, 500, 1000, 500], + expected_remaining_power=0.0, ) - self.assert_result( + self.assert_any_result( algorithm.distribute_power(3500, components), - DistributionResult( - {10: 1000, 11: 500, 20: 1000, 21: 500}, remaining_power=500.0 - ), + expected_distribution_ids={10, 11, 20, 21}, + expected_distribution_powers=[1000, 500, 1000, 500], + expected_remaining_power=500.0, ) diff --git a/tests/timeseries/_battery_pool/test_battery_pool.py b/tests/timeseries/_battery_pool/test_battery_pool.py index b19a5a08b..5123f759e 100644 --- a/tests/timeseries/_battery_pool/test_battery_pool.py +++ b/tests/timeseries/_battery_pool/test_battery_pool.py @@ -198,11 +198,21 @@ async def setup_batteries_pool(mocker: MockerFixture) -> AsyncIterator[SetupArgs # the scope of this tests. This tests should cover BatteryPool only. # We use our own battery status channel, where we easily control set of working # batteries. - all_batteries = list(get_components(mock_microgrid, ComponentCategory.BATTERY)) + all_batteries = get_components(mock_microgrid, ComponentCategory.BATTERY) - battery_pool = microgrid.new_battery_pool( - priority=5, component_ids=set(all_batteries[:2]) - ) + # This is a hack because these tests used to rely on the order in which components + # are returned from the component graph using `all_batteries[:2]` to get the first 2 + # batteries. But the component graph returns a set, which doesn't have any ordering + # guarantees, so if the Python implementation changes, the hashing can change and + # this resulting order could be different, breaking the tests. This is why we are + # now specifying IDs to use explicitly (these are the IDs that were used when the + # tests were written). This can also change in the future if generator of mock + # components changes, as it might produce different IDs, so this solution is also + # not bullet-proof. + assert 8 in all_batteries + assert 11 in all_batteries + + battery_pool = microgrid.new_battery_pool(priority=5, component_ids=set([8, 11])) dp = microgrid._data_pipeline._DATA_PIPELINE assert dp is not None @@ -1125,7 +1135,7 @@ async def run_power_bounds_test( # pylint: disable=too-many-locals ) compare_messages(msg, expected) - batteries_in_pool = list(battery_pool.component_ids) + batteries_in_pool = list(sorted(battery_pool.component_ids)) scenarios: list[Scenario[SystemBounds]] = [ Scenario( next(iter(bat_invs_map[batteries_in_pool[0]])), diff --git a/tests/timeseries/test_formula_formatter.py b/tests/timeseries/test_formula_formatter.py index 784c15f8c..f95c4ab3b 100644 --- a/tests/timeseries/test_formula_formatter.py +++ b/tests/timeseries/test_formula_formatter.py @@ -4,15 +4,13 @@ """Tests for the FormulaFormatter.""" -from contextlib import AsyncExitStack - from frequenz.channels import Broadcast from frequenz.quantities import Quantity -from pytest_mock import MockerFixture -from frequenz.sdk import microgrid from frequenz.sdk.timeseries import Sample -from frequenz.sdk.timeseries.formula_engine._formula_engine import FormulaBuilder +from frequenz.sdk.timeseries.formula_engine._formula_engine import ( + FormulaBuilder, +) from frequenz.sdk.timeseries.formula_engine._formula_formatter import format_formula from frequenz.sdk.timeseries.formula_engine._formula_steps import ( Clipper, @@ -22,7 +20,6 @@ Minimizer, ) from frequenz.sdk.timeseries.formula_engine._tokenizer import Tokenizer, TokenType -from tests.timeseries.mock_microgrid import MockMicrogrid def build_formula(formula: str) -> list[FormulaStep]: @@ -112,27 +109,34 @@ def test_functions(self) -> None: # flake8: enable # fmt: on - async def test_higher_order_formula(self, mocker: MockerFixture) -> None: - """Test that the formula is formatted correctly for a higher-order formula.""" - mockgrid = MockMicrogrid(grid_meter=False, mocker=mocker) - mockgrid.add_batteries(3) - mockgrid.add_ev_chargers(1) - mockgrid.add_solar_inverters(2) - - async with mockgrid, AsyncExitStack() as stack: - logical_meter = microgrid.logical_meter() - stack.push_async_callback(logical_meter.stop) - - pv_pool = microgrid.new_pv_pool(priority=5) - stack.push_async_callback(pv_pool.stop) - - grid = microgrid.grid() - stack.push_async_callback(grid.stop) - - assert str(grid.power) == "#36 + #7 + #47 + #17 + #57 + #27" - - composed_formula = (grid.power - pv_pool.power).build("grid_minus_pv") - assert ( - str(composed_formula) - == "[grid-power](#36 + #7 + #47 + #17 + #57 + #27) - [pv-power](#47 + #57)" - ) + async def test_higher_order_formula(self) -> None: + """Test that higher-order formulas (formulas combining other formulas) are formatted correctly.""" + # Create two base formulas + builder1 = FormulaBuilder("test_formula1", Quantity) + builder2 = FormulaBuilder("test_formula2", Quantity) + + # Push metrics directly to the builders + channel1 = Broadcast[Sample[Quantity]](name="channel1") + channel2 = Broadcast[Sample[Quantity]](name="channel2") + builder1.push_metric("#1", channel1.new_receiver(), nones_are_zeros=True) + builder1.push_oper("+") + builder1.push_metric("#2", channel2.new_receiver(), nones_are_zeros=True) + + channel3 = Broadcast[Sample[Quantity]](name="channel3") + channel4 = Broadcast[Sample[Quantity]](name="channel4") + builder2.push_metric("#3", channel3.new_receiver(), nones_are_zeros=True) + builder2.push_oper("+") + builder2.push_metric("#4", channel4.new_receiver(), nones_are_zeros=True) + + # Build individual formula engines first + engine1 = builder1.build() + engine2 = builder2.build() + + # Combine them into a higher-order formula + composed_formula = (engine1 - engine2).build("higher_order_formula") + + # Check the string representation + assert ( + str(composed_formula) + == "[test_formula1](#1 + #2) - [test_formula2](#3 + #4)" + )