Fix `consumer_power()` not working certain configurations. #589

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

@@ -70,7 +70,7 @@ async def benchmark_data_sourcing(
        num_ev_chargers * len(COMPONENT_METRIC_IDS) * num_msgs_per_battery
    )
    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

@@ -62,7 +62,7 @@ def __init__(  # pylint: disable=too-many-arguments
        """Create a new instance.

        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

@@ -88,7 +88,7 @@ async def test_grid_power_2(
        results: list[Quantity] = []
        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

@@ -62,7 +62,7 @@ async def test_grid_power_2(
        mocker: MockerFixture,
    ) -> 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...

Bumps [pytest-cov](https://github.com/pytest-dev/pytest-cov) from 4.0.0 to 4.1.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst">pytest-cov's changelog</a>.</em></p> <blockquote> <h2>4.1.0 (2023-05-24)</h2> <ul> <li>Updated CI with new Pythons and dependencies.</li> <li>Removed rsyncdir support. This makes pytest-cov compatible with xdist 3.0. Contributed by Sorin Sbarnea in <code>[frequenz-floss#558](pytest-dev/pytest-cov#558) <https://github.com/pytest-dev/pytest-cov/pull/558></code>_.</li> <li>Optimized summary generation to not be performed if no reporting is active (for example, when <code>--cov-report=''</code> is used without <code>--cov-fail-under</code>). Contributed by Jonathan Stewmon in <code>[frequenz-floss#589](pytest-dev/pytest-cov#589) <https://github.com/pytest-dev/pytest-cov/pull/589></code>_.</li> <li>Added support for JSON reporting. Contributed by Matthew Gamble in <code>[frequenz-floss#582](pytest-dev/pytest-cov#582) <https://github.com/pytest-dev/pytest-cov/pull/582></code>_.</li> <li>Refactored code to use f-strings. Contributed by Mark Mayo in <code>[frequenz-floss#572](pytest-dev/pytest-cov#572) <https://github.com/pytest-dev/pytest-cov/pull/572></code>_.</li> <li>Fixed a skip in the test suite for some old xdist. Contributed by a bunch of people in <code>[frequenz-floss#565](pytest-dev/pytest-cov#565) <https://github.com/pytest-dev/pytest-cov/pull/565></code>_.</li> </ul> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/2c9f2170d8575b21bafb6402eb30ca7de31e20b9"><code>2c9f217</code></a> Bump version: 4.0.0 → 4.1.0</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/4d245df8f75e434a9e1b162b51265d6a45017465"><code>4d245df</code></a> Update changelog and authors.</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/7b095c84ae521b14058d7d520ef36372849063a8"><code>7b095c8</code></a> Skip starting from xdist 3.0.2 (where boxed was removed).</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/605d6902b3b3d17aad0bf811b8c580fc895724ca"><code>605d690</code></a> disabling boxed test if version xdist newer than 2.5.0</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/76fb2a6cb2c4a4a788a5b62710848daf9c8fb7ce"><code>76fb2a6</code></a> introduced f-strings</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/0d63ede0d2ca9f4acc8329aa4261a7cec489ffdb"><code>0d63ede</code></a> Update test config. Reapply some of the changes from PR567 to the right file ...</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/f3d8d8380f6a4b265353fe7cd509b040702f1e64"><code>f3d8d83</code></a> Add support for JSON reporter</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/dec02abeb9fa8ee3547baa054bde6006bea530ee"><code>dec02ab</code></a> Update test deps.</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/88a7d348986bace58e26c88a713ef35f900ce2ef"><code>88a7d34</code></a> chore: update AUTHORS and CHANGELOG</li> <li><a href="https://github.com/pytest-dev/pytest-cov/commit/74eb4cc8b684269b89735e31b623f0f9795c5d5c"><code>74eb4cc</code></a> perf: only call summary when the report will be used</li> <li>Additional commits viewable in <a href="https://github.com/pytest-dev/pytest-cov/compare/v4.0.0...v4.1.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pytest-cov&package-manager=pip&previous-version=4.0.0&new-version=4.1.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>

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 <mathias.baumann@frequenz.com>

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

In microgrids without consumers and no main meter, the formula would never return any values. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

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 <mathias.baumann@frequenz.com>

Add some tests for scenarios with no meter.

0f8063f

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

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 <mathias.baumann@frequenz.com>

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
9 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `consumer_power()` not working certain configurations. #589

Fix `consumer_power()` not working certain configurations. #589

Marenz commented Aug 18, 2023 •

edited

matthias-wende-frequenz Aug 20, 2023

shsms Aug 21, 2023

Marenz Aug 21, 2023

shsms Aug 21, 2023

shsms Aug 21, 2023

matthias-wende-frequenz Aug 20, 2023

matthias-wende-frequenz Aug 20, 2023

Marenz Aug 21, 2023

matthias-wende-frequenz Aug 21, 2023

Marenz Aug 21, 2023

matthias-wende-frequenz Aug 20, 2023

matthias-wende-frequenz Aug 20, 2023

Marenz Aug 21, 2023

Marenz commented Aug 21, 2023

shsms left a comment

shsms left a comment

Fix consumer_power() not working certain configurations. #589

Fix consumer_power() not working certain configurations. #589

Conversation

Marenz commented Aug 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marenz commented Aug 21, 2023

shsms left a comment

Choose a reason for hiding this comment

shsms left a comment

Choose a reason for hiding this comment

Fix `consumer_power()` not working certain configurations. #589

Fix `consumer_power()` not working certain configurations. #589

Marenz commented Aug 18, 2023 •

edited