Support exclusion bounds in power distributor by shsms · Pull Request #562 · frequenz-floss/frequenz-sdk-python

shsms · 2023-08-03T17:08:06Z

To be merged after #537

Closes #152

shsms · 2023-08-03T17:09:52Z

Still need to add tests.

This method will replace the `_get_{upper,lower}_bound` methods in subsequent commits. Signed-off-by: Sahas Subramanian <[email protected]>

Signed-off-by: Sahas Subramanian <[email protected]>

This just updates the tests to use data structures that support exclusion bounds, so that once support is implemented in the actor, it would be easy to add incremental tests. Signed-off-by: Sahas Subramanian <[email protected]>

Signed-off-by: Sahas Subramanian <[email protected]>

This allows us to not pack all tests in the same class, but group them separately in smaller units. Signed-off-by: Sahas Subramanian <[email protected]>

Signed-off-by: Sahas Subramanian <[email protected]>

llucax

Except for the abs_tol for deficit (which I'm not sure it is an issue or not), nothing major from my side. I didn't dig very deep into the distribution algorithm itself. The rest can be easily applied by accepting the suggestions.

src/frequenz/sdk/actor/power_distributing/power_distributing.py

tests/actor/test_power_distributing.py

src/frequenz/sdk/power/_distribution_algorithm.py

llucax · 2023-08-08T11:30:25Z

src/frequenz/sdk/power/_distribution_algorithm.py

+                if math.isclose(take_from[1], 0.0, abs_tol=1e-6) or take_from[1] < 0.0:
+                    break
+                if take_from[1] >= -deficit or math.isclose(
+                    take_from[1], -deficit, abs_tol=1e-6


I deficit supposed to be close to zero? abs_tol seems to be mainly useful for comparing to near-zero values AFAIK, not sure if it is generally OK to use it for other values or not. Maybe @matthias-wende-frequenz can speak his mathematician mind...

This is a >=. But the = part is not exact, value is sometimes slightly on the other side. hence the isclose.

Yeah, what I'm asking if it is OK is specifically the abs_tol argument.

I don't understand the full algorithm but the check is fine. It's just about having a higher certainty the the deficit is close the excess_reserved under consideration (the take_from[1]).

I think this should use the relative tolerance instead as take_from[1] and deficit are already compared to zero above where the absolute tolerance makes more sense

The relative tolerance is defined by default (rel_tol=1e-09), this is why I asked only about removing tol_abs.

src/frequenz/sdk/actor/power_distributing/power_distributing.py

daniel-zullo-frequenz · 2023-08-08T20:41:57Z

src/frequenz/sdk/power/_distribution_algorithm.py

+                if left_over > -deficit:
+                    distributed_power += deficit
+                    deficit = 0.0
+                    deficits[inverter_id] = 0.0


I think deficits does not need to be updated in this for-loop

true, only the distributed_power needs to be updated. I guess I added the other two just to be able to properly reason about what was going on as I was developing, but they don't have to be there.

Maybe add a comment about it in the refactor PR if it would make reasoning about it easier.

daniel-zullo-frequenz · 2023-08-08T20:42:56Z

src/frequenz/sdk/power/_distribution_algorithm.py

+                elif left_over > 0.0:
+                    deficit += left_over
+                    distributed_power += left_over
+                    deficits[inverter_id] = deficit


Updating deficits might be redundant here as it will no longer be used after the for-loop

daniel-zullo-frequenz · 2023-08-08T22:40:58Z

src/frequenz/sdk/power/_distribution_algorithm.py

+            distribution[inverter_id] += excess
+            distributed_power += excess
+
+        for inverter_id, deficit in deficits.items():


This duplicated for-loop is not needed as its content could be added in the first loop for deficits, right after the end of the while-loop

Signed-off-by: Leandro Lucarella <[email protected]>

We already have an utility function to check if a `float` is close to zero, so we can use that instead. Signed-off-by: Leandro Lucarella <[email protected]>

Co-authored-by: daniel-zullo-frequenz <[email protected]> Signed-off-by: Leandro Lucarella <[email protected]>

llucax · 2023-08-09T12:50:27Z

Refactor deficit calculation in the power distributor #572

- [x] Remove absolute tolerance in floating-point comparison: see #562 (comment) for more details - [x] Remove redundant deficits updates: see #562 (comment) for more details - [x] Consolidate deficits for-loops: see #562 (comment) for more details This is an internal refactoring for the power distribution algorithm maintaining its functionality, so release notes don't need to be updated. Fixes #572

shsms added this to the v0.24.0 milestone Aug 3, 2023

shsms requested a review from a team as a code owner August 3, 2023 17:08

shsms requested a review from matthias-wende-frequenz August 3, 2023 17:08

shsms marked this pull request as draft August 3, 2023 17:08

shsms force-pushed the power-dist-excl-bounds branch 2 times, most recently from 8df671c to 2200e1b Compare August 4, 2023 17:24

github-actions bot removed the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Aug 4, 2023

shsms force-pushed the power-dist-excl-bounds branch from 2200e1b to 6caeb7b Compare August 4, 2023 17:37

llucax assigned shsms Aug 8, 2023

shsms added 8 commits August 8, 2023 12:22

Add a _get_bounds method in PowerDistributingActor

d574ae3

This method will replace the `_get_{upper,lower}_bound` methods in subsequent commits. Signed-off-by: Sahas Subramanian <[email protected]>

Remove unused to_protobuf method in power distribution tests

d430343

Signed-off-by: Sahas Subramanian <[email protected]>

Support exclusion bounds in power distribution tests

db23e32

This just updates the tests to use data structures that support exclusion bounds, so that once support is implemented in the actor, it would be easy to add incremental tests. Signed-off-by: Sahas Subramanian <[email protected]>

Respect exclusion bounds in PowerDistributingActor._check_request

12749c1

Signed-off-by: Sahas Subramanian <[email protected]>

Refactor to reduce the amount of calls to the component data cache

de1331d

Signed-off-by: Sahas Subramanian <[email protected]>

Update distribution algorithm to support exclusion bounds

3ee0f27

Signed-off-by: Sahas Subramanian <[email protected]>

Extract reusable create_components method outside test class

ffb95bf

This allows us to not pack all tests in the same class, but group them separately in smaller units. Signed-off-by: Sahas Subramanian <[email protected]>

Test exclusion bounds in power distribution algorithm

03a3cae

Signed-off-by: Sahas Subramanian <[email protected]>

shsms force-pushed the power-dist-excl-bounds branch from 6caeb7b to 03a3cae Compare August 8, 2023 10:23

github-actions bot removed part:docs Affects the documentation part:data-pipeline Affects the data pipeline part:microgrid Affects the interactions with the microgrid labels Aug 8, 2023

shsms marked this pull request as ready for review August 8, 2023 10:23

Update RELEASE_NOTES.md

390096c

Signed-off-by: Sahas Subramanian <[email protected]>

github-actions bot added the part:docs Affects the documentation label Aug 8, 2023

llucax removed this from the v0.24.0 milestone Aug 8, 2023

llucax reviewed Aug 8, 2023

View reviewed changes

daniel-zullo-frequenz reviewed Aug 8, 2023

View reviewed changes

llucax and others added 3 commits August 9, 2023 10:07

Remove leftover print()

c3418aa

Signed-off-by: Leandro Lucarella <[email protected]>

Use is_close_to_zero() instead of math.isclose(..., abs_tol=...)

f034cc4

We already have an utility function to check if a `float` is close to zero, so we can use that instead. Signed-off-by: Leandro Lucarella <[email protected]>

Simplify a < b < c comparison

be9d239

Co-authored-by: daniel-zullo-frequenz <[email protected]> Signed-off-by: Leandro Lucarella <[email protected]>

daniel-zullo-frequenz approved these changes Aug 9, 2023

View reviewed changes

llucax added this pull request to the merge queue Aug 9, 2023

Merged via the queue into frequenz-floss:v0.x.x with commit 6258c76 Aug 9, 2023

llucax mentioned this pull request Aug 9, 2023

Refactor deficit calculation in the power distributor #572

Closed

shsms deleted the power-dist-excl-bounds branch August 9, 2023 13:20

daniel-zullo-frequenz mentioned this pull request Aug 10, 2023

Amend deficit calculation in the power distributor #577

Merged

3 tasks

Comments

Conversation

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

Uh oh!

Uh oh!

shsms commented Aug 3, 2023

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

daniel-zullo-frequenz Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

llucax commented Aug 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shsms commented Aug 3, 2023 •

edited

Loading

daniel-zullo-frequenz Aug 9, 2023 •

edited

Loading