Battery pool bounds are not calculated correctly

[llucax]

What happened?

In tests/timeseries/_battery_pool/test_battery_pool.py, test_all_batteries_power_bounds, the first test scenario is something like this:

    for battery_id, inverter_ids in bat_invs_map.items():
        # Sampling rate choose to reflect real application.
        streamer.start_streaming(
            BatteryDataWrapper(
                component_id=battery_id,
                timestamp=datetime.now(tz=timezone.utc),
                power_inclusion_lower_bound=-1000,
                power_inclusion_upper_bound=5000,
                power_exclusion_lower_bound=-300,
                power_exclusion_upper_bound=300,
            ),
            sampling_rate=0.05,
        )
        for inverter_id in inverter_ids:
            streamer.start_streaming(
                InverterDataWrapper(
                    component_id=inverter_id,
                    timestamp=datetime.now(tz=timezone.utc),
                    active_power_inclusion_lower_bound=-900,
                    active_power_inclusion_upper_bound=6000,
                    active_power_exclusion_lower_bound=-200,
                    active_power_exclusion_upper_bound=200,
                ),
                sampling_rate=0.1,
            )

    # pylint: disable=protected-access
    receiver = battery_pool._system_power_bounds.new_receiver(limit=50)
    # pylint: enable=protected-access

    # First metrics delivers slower because of the startup delay in the pool.
    msg = await asyncio.wait_for(
        receiver.receive(), timeout=WAIT_FOR_COMPONENT_DATA_SEC + 0.2
    )
    now = datetime.now(tz=timezone.utc)
    expected = SystemBounds(
        timestamp=now,
        inclusion_bounds=Bounds(Power.from_watts(-1800), Power.from_watts(10000)),
        exclusion_bounds=Bounds(Power.from_watts(-600), Power.from_watts(600)),
    )
    compare_messages(msg, expected)

    batteries_in_pool = list(battery_pool.component_ids)

    scenarios: list[Scenario[SystemBounds]] = [
        Scenario(
            next(iter(bat_invs_map[batteries_in_pool[0]])),
            {
                "active_power_inclusion_lower_bound": -100,
                "active_power_exclusion_lower_bound": -400,
            },
            SystemBounds(
                timestamp=now,
                inclusion_bounds=Bounds(Power.from_watts(-1000), Power.from_watts(10000)),
                exclusion_bounds=Bounds(Power.from_watts(-700), Power.from_watts(600)),
            ),
        ),

Adding some logging, this is the result of the initial state (we have battery 5 with inverter 4 and battery 8 with inverter 7):

INFO     frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:545 working_batteries={8, 5}
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:591 >>> battery_ids=frozenset({5}) inverter_ids=frozenset({4}) battery_bounds=[PowerBounds(inclusion_lower=-1000, exclusion_lower=-300, exclusion_upper=300, inclusion_upper=5000)]
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:596 >>> aggregated_bat_bounds=PowerBounds(inclusion_lower=-1000, exclusion_lower=-300, exclusion_upper=300, inclusion_upper=5000)
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:599 >>> inverter_bounds=[PowerBounds(inclusion_lower=-900, exclusion_lower=-200, exclusion_upper=200, inclusion_upper=6000)]
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:591 >>> battery_ids=frozenset({8}) inverter_ids=frozenset({7}) battery_bounds=[PowerBounds(inclusion_lower=-1000, exclusion_lower=-300, exclusion_upper=300, inclusion_upper=5000)]
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:596 >>> aggregated_bat_bounds=PowerBounds(inclusion_lower=-1000, exclusion_lower=-300, exclusion_upper=300, inclusion_upper=5000)
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:599 >>> inverter_bounds=[PowerBounds(inclusion_lower=-900, exclusion_lower=-200, exclusion_upper=200, inclusion_upper=6000)]
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:649 Calculated Power Bounds: inclusion=[-1800.0, 10000.0] exclusion=[-600.0, 600.0]

So we have the same data for both battery chains, that look like:

             ____________________ incl ________________________________________
            |                                                                  |
INV   ------|xxxxxxxxxxx|---------|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|-----
         -900        -200       200                                           6000
           :           |_ excl __|
           :
          ______________________ incl ____________________________________
         | :                                                              |
BAT  ----|xxxxxxxxxxx|-------------|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|----------
       -1000        -300          300                                   5000
           :         |___ excl ____|                                      :
           :         :             :                                      :
           :         :             :                                      :
           :____________________ incl ____________________________________:
           |         :             :                                      |
RES  ------|xxxxxxxxx|-------------|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|----------
         -900       -300          300                                   5000
                     |___ excl ____|

So the end result is twice these bounds, as it is one per battery chain:

incl = [-1_800, 10_000]
excl = [-600, 600]

For the first test scenario, the inverter 7 (for battery 8) gets new bounds:

inclusion_lower=-100, exclusion_lower=-400, exclusion_upper=200, inclusion_upper=6000

Note that the exclusion bounds are not contained in the inclusion bounds.

This is the extra info for the tests:

INFO     frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:545 working_batteries={8, 5}
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:591 >>> battery_ids=frozenset({5}) inverter_ids=frozenset({4}) battery_bounds=[PowerBounds(inclusion_lower=-1000, exclusion_lower=-300, exclusion_upper=300, inclusion_upper=5000)]
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:596 >>> aggregated_bat_bounds=PowerBounds(inclusion_lower=-1000, exclusion_lower=-300, exclusion_upper=300, inclusion_upper=5000)
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:599 >>> inverter_bounds=[PowerBounds(inclusion_lower=-900, exclusion_lower=-200, exclusion_upper=200, inclusion_upper=6000)]
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:591 >>> battery_ids=frozenset({8}) inverter_ids=frozenset({7}) battery_bounds=[PowerBounds(inclusion_lower=-1000, exclusion_lower=-300, exclusion_upper=300, inclusion_upper=5000)]
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:596 >>> aggregated_bat_bounds=PowerBounds(inclusion_lower=-1000, exclusion_lower=-300, exclusion_upper=300, inclusion_upper=5000)
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:599 >>> inverter_bounds=[PowerBounds(inclusion_lower=-100, exclusion_lower=-400, exclusion_upper=200, inclusion_upper=6000)]
WARNING  frequenz.sdk.timeseries.battery_pool._metric_calculator:_metric_calculator.py:649 Calculated Power Bounds: inclusion=[-1000.0, 10000.0] exclusion=[-700.0, 600.0]

So the initial data is:

And this is how this looks like:

             ____________________ incl ________________________________________
            |                                                                  |
INV4  ------|xxxxxxxxxxx|---------|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|-----
         -900        -200       200                                           6000
           :           |_ excl __|
           :
          ______________________ incl ____________________________________
         | :                                                              |
BAT5 ----|xxxxxxxxxxx|-------------|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|----------
       -1000        -300          300                                   5000
           :         |___ excl ____|                                      :
           :         :             :                                      :
           :         :             :                                      :
           :____________________ incl ____________________________________:
           |         :             :                                      |
RES  ------|xxxxxxxxx|-------------|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|----------
         -900       -300          300                                   5000
                     |___ excl ____|

                           ____________________ incl __________________________
                          |                                                    |
INV7  -----------|--------|------|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|-----
               -400     -100    200                                           6000
                 |_____ excl ____|

          ______________________ incl ____________________________________
         |                                                                |
BAT8 ----|xxxxxxxxxxx|-------------|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|----------
       -1000        -300          300                                   5000
                     |___ excl ____|                                      :
                                   :                                      :
                                   :                                      :
                                   :__________________ incl ______________:
                                   :                                      |
RES  ------------------------------|xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx|----------
                                  300                                   5000

So the resulting bounds for each battery chain should be:

bat5_incl_lower = -900
bat5_excl_lower = -300
bat5_excl_upper = 300
bat5_incl_upper = 5000

bat8_incl_lower = 300
# no excl bounds
bat8_incl_upper = 5000

So the total resulting bounds should be:

incl_lower = (-900) + (300) = -600
excl_lower = (-300) + (300) = 0
excl_upper = (300) + (300) = 600
incl_upper = (5000) + (5000) = 10000

Nevertheless, the battery pool is producing:

incl_lower = -1000
excl_lower = -700
excl_upper = 600
incl_upper = 10000

And the tests are validating this wrong result.

Extra information

The problem seems to manifest when the exclusion bounds are not contained inside the inclusion bounds, the algorithm seems to be very naive and just do:

incl_lower = max(Battery inclusion_lower, Inverter inclusion_lower)
incl_upper = min(Battery inclusion_upper, Inverter inclusion_upper)
excl_lower = min(Battery exclusion_lower, Inverter exclusion_lower)
excl_upper = max(Battery exclusion_upper, Inverter exclusion_upper)

Which doesn't really work when there is effectively no exclusion bounds for some case.

This was discovered while migrating to the microgrid API v0.17, which uses a set of allowed ranges instead a pair of inclusion/exclusion bounds.

Also when working with allowed sets, the calculation should be much more straightforward, and probably handling edge cases will have more predictable results. For example now if we have something like: incl = [100, 1000]; excl = [100, 200], is 100 an allowed value or not? This is not clear. With ranges, this can be clearly represented as [100, 100], [200, 1000] or just [200, 1000] depending on if 100 is allowed or not.