Fix `consumer_power()` not working certain configurations. by Marenz · Pull Request #589 · frequenz-floss/frequenz-sdk-python

Marenz · 2023-08-18T09:02:52Z

Fix MockMicrogrid bug not properly disabling main meter
Fix consumer_power() not working certain configurations.
In microgrids without consumers and no main meter, the formula
would never return any values.
This was undiscovered as we didn't test that scenario in the MockMicrogrid class used
for testing.

matthias-wende-frequenz · 2023-08-20T20:19:38Z

benchmarks/timeseries/benchmark_datasourcing.py

    )
    mock_grid = MockMicrogrid(
-        grid_side_meter=False, num_values=num_msgs_per_battery, sample_rate_s=0.0
+        main_meter=False, num_values=num_msgs_per_battery, sample_rate_s=0.0


I suggest to call it grid_meter as it's the successor to the GRID component. The term main_meter isn't helpful w.r.t. the meter placement.

if you're adding a add_consumer_meters, then you might as well add a add_grid_meters method and drop the flag. There has already been some talk about locations with multiple grid meters anyway.

that' s the next step actually

I feel like this is not a good idea, because this makes our tests rely on the implementation rather than on just the expected behaviour. I think instead we should always add a main meter, and have special cases for graphs without main meters. Maybe we can discuss tomorrow during the meeting.

I have added it to the agenda.

matthias-wende-frequenz · 2023-08-20T20:24:43Z

tests/timeseries/mock_microgrid.py


        Args:
-            grid_side_meter: whether the main meter should be on the grid side or not.
+            main_meter: whether there is a main meter that is between grid and microgrid.


whether there is a meter successor of the GRID component.

matthias-wende-frequenz · 2023-08-20T20:47:51Z

tests/timeseries/test_logical_meter.py

        meter_sums: list[Quantity] = []
        for count in range(10):
-            await mockgrid.mock_resampler.send_meter_power([20.0 + count, 12.0, -13.0])
+            await mockgrid.mock_resampler.send_meter_power([20.0 + count, 12.0])


shouldn't the first value in the list send to the non existing meter?

it doesn't really matter which values are used here as we simply sum them up and compare

After discussing with Sahas, we should add a consumption meter here, because we are not testing the same case any longer.

matthias-wende-frequenz · 2023-08-20T20:57:24Z

tests/timeseries/test_logical_meter.py

    ) -> None:
        """Test the grid power formula without a grid side meter."""
-        mockgrid = MockMicrogrid(grid_side_meter=False)
+        mockgrid = MockMicrogrid(main_meter=False)


Although one meter is removed, this test still works, since we compare the logical_meters power against the sum of all the other meters.

matthias-wende-frequenz · 2023-08-20T21:18:06Z

tests/timeseries/test_logical_meter.py

+        grid_production_recv = logical_meter.grid_production_power.new_receiver()
+        grid_consumption_recv = logical_meter.grid_consumption_power.new_receiver()
+
+        await mockgrid.mock_resampler.send_meter_power([2.5, 3.5, 4.0])


A Solar inverter that charges the PV panels? Interesting case 😂.

It's the same power sum as the test above...

Marenz · 2023-08-21T14:11:22Z

updated

shsms

I have some comments: #589 (comment)

blocking until we discuss tomorrow.

Renames the `grid_side_meter` parameter to be more understandable and explicit. Signed-off-by: Mathias L. Baumann <[email protected]>

Signed-off-by: Mathias L. Baumann <[email protected]>

In microgrids without consumers and no main meter, the formula would never return any values. Signed-off-by: Mathias L. Baumann <[email protected]>

shsms

It turns out my big objections were incorrect, I guess we just have to write more granular test cases in the future, so unblocking.

Marenz requested a review from a team as a code owner August 18, 2023 09:02

Marenz requested a review from shsms August 18, 2023 09:02

Marenz self-assigned this Aug 18, 2023

github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline part:actor Affects an actor ot the actors utilities (decorator, etc.) labels Aug 18, 2023

Marenz added type:bug Something isn't working and removed part:actor Affects an actor ot the actors utilities (decorator, etc.) labels Aug 18, 2023

Marenz force-pushed the tmp branch from 981e8ef to 0eba006 Compare August 18, 2023 09:05

github-actions bot added part:docs Affects the documentation part:actor Affects an actor ot the actors utilities (decorator, etc.) labels Aug 18, 2023

Marenz force-pushed the tmp branch 2 times, most recently from 23ee961 to d0cea85 Compare August 18, 2023 11:19

github-actions bot removed the part:actor Affects an actor ot the actors utilities (decorator, etc.) label Aug 18, 2023

Marenz force-pushed the tmp branch from d0cea85 to 81b9d5c Compare August 18, 2023 11:29

Marenz requested a review from matthias-wende-frequenz August 18, 2023 11:29

matthias-wende-frequenz reviewed Aug 20, 2023

View reviewed changes

Marenz force-pushed the tmp branch from 81b9d5c to 5e3d475 Compare August 21, 2023 12:25

matthias-wende-frequenz previously approved these changes Aug 21, 2023

View reviewed changes

Marenz added this pull request to the merge queue Aug 21, 2023

shsms removed this pull request from the merge queue due to a manual request Aug 21, 2023

shsms requested changes Aug 21, 2023

View reviewed changes

Marenz dismissed matthias-wende-frequenz’s stale review via 9b45bb0 August 21, 2023 15:44

Marenz force-pushed the tmp branch from 5e3d475 to 9b45bb0 Compare August 21, 2023 15:44

Marenz added 3 commits August 21, 2023 17:45

MockMicrogrid: Add a consumer meter only explicitly.

2e6ca85

Renames the `grid_side_meter` parameter to be more understandable and explicit. Signed-off-by: Mathias L. Baumann <[email protected]>

Add some tests for scenarios with no meter.

0f8063f

Signed-off-by: Mathias L. Baumann <[email protected]>

Fix consumer_power() not working certain configurations.

14f4cd8

In microgrids without consumers and no main meter, the formula would never return any values. Signed-off-by: Mathias L. Baumann <[email protected]>

Marenz force-pushed the tmp branch from 9b45bb0 to 14f4cd8 Compare August 21, 2023 15:45

shsms approved these changes Aug 21, 2023

View reviewed changes

llucax added this to the v0.25.0 milestone Aug 22, 2023

matthias-wende-frequenz approved these changes Aug 22, 2023

View reviewed changes

Marenz added this pull request to the merge queue Aug 22, 2023

Merged via the queue into frequenz-floss:v0.x.x with commit cac5473 Aug 22, 2023

Marenz deleted the tmp branch August 22, 2023 13:29

Comments

Conversation

Marenz commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marenz commented Aug 21, 2023

Uh oh!

Uh oh!

shsms left a comment

Choose a reason for hiding this comment

Uh oh!

shsms left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Marenz commented Aug 18, 2023 •

edited

Loading