Support exclusion bounds in power distributor #562
Conversation
github-actions
bot
added
part:docs
|
Still need to add tests. |
shsms
force-pushed
the
power-dist-excl-bounds
branch
2 times, most recently
from
8df671c
to
2200e1b
Compare
github-actions
bot
removed
the
part:tooling
shsms
force-pushed
the
power-dist-excl-bounds
branch
from
2200e1b
to
6caeb7b
Compare
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]>
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]>
shsms
force-pushed
the
power-dist-excl-bounds
branch
from
6caeb7b
to
03a3cae
Compare
github-actions
bot
removed
part:docs
Signed-off-by: Sahas Subramanian <[email protected]>
| 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 |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
This is a >=. But the = part is not exact, value is sometimes slightly on the other side. hence the isclose.
There was a problem hiding this comment.
Yeah, what I'm asking if it is OK is specifically the abs_tol argument.
There was a problem hiding this comment.
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]).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
| if left_over > -deficit: | ||
| distributed_power += deficit | ||
| deficit = 0.0 | ||
| deficits[inverter_id] = 0.0 |
There was a problem hiding this comment.
I think deficits does not need to be updated in this for-loop
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe add a comment about it in the refactor PR if it would make reasoning about it easier.
| elif left_over > 0.0: | ||
| deficit += left_over | ||
| distributed_power += left_over | ||
| deficits[inverter_id] = deficit |
There was a problem hiding this comment.
Updating deficits might be redundant here as it will no longer be used after the for-loop
| distribution[inverter_id] += excess | ||
| distributed_power += excess | ||
|
|
||
| for inverter_id, deficit in deficits.items(): |
There was a problem hiding this comment.
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]>
- [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
To be merged after #537
Closes #152