Stream exclusion bounds from the battery pool #537

shsms · 2023-07-26T13:23:39Z

Based on #416, to be merged after upgrading the Microgrid API dependency to v0.15.1.

Also closes #524

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

It is a class that contains lower and upper bounds, so plural is better. There are other breaking changes related to inclusion and exclusion bounds, thar are coming up in subsequent commits, so now is a nice opportunity to make this change. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

`inclusion/exclusion` bounds are a better representation of the passive sign convention, than `supply/consume` bounds. But `supply/consume` bounds are not gone for ever, they will be re-implemented as unsigned `charge/discharge` bounds in a separate PR. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

They were represented with `float`s before. Closes frequenz-floss#524 Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

This is because once we add exclusion bounds, the function will become too big. And instead, we can add it as a separate method in a subsequent commit. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

shsms · 2023-08-04T16:46:27Z

Rebased on latest

llucax

Only a few very minor comments that can be addressed in a separate PR.

llucax · 2023-08-08T07:44:06Z

src/frequenz/sdk/timeseries/battery_pool/_result_types.py

+    When exclusion bounds are present, they will exclude a subset of the inclusion
+    bounds.
+
+    More details [here](https://github.com/frequenz-floss/frequenz-api-common/blob/v0.3.0/proto/frequenz/api/common/metrics.proto#L37-L91).


This could now point to the generated docs: https://frequenz-floss.github.io/frequenz-api-common/next/python-reference/frequenz/api/common/metrics_pb2/#frequenz.api.common.metrics_pb2.Metric.system_exclusion_bounds

We could even just use python symbols for cross referencing if we add these object indexes:

diff --git a/mkdocs.yml b/mkdocs.yml index e8ddb46..13fe0f1 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -105,7 +105,9 @@ plugins: import: # See https://mkdocstrings.github.io/python/usage/#import for details - https://docs.python.org/3/objects.inv + - https://frequenz-floss.github.io/frequenz-api-common/v0.3/objects.inv - https://frequenz-floss.github.io/frequenz-channels-python/v0.14/objects.inv + - https://frequenz-floss.github.io/frequenz-api-microgrid/v0.15/objects.inv - https://grpc.github.io/grpc/python/objects.inv - https://networkx.org/documentation/stable/objects.inv - https://numpy.org/doc/stable/objects.inv

Suggested change

More details [here](https://github.com/frequenz-floss/frequenz-api-common/blob/v0.3.0/proto/frequenz/api/common/metrics.proto#L37-L91).

See [`frequenz.api.common.metrics_pb2.Metric.system_exclusion_bounds`][] for more details.

But for this to work we need to initialize the microgrid API generated docs and we need to tag v0.3.1 in the common API so we can have a stable link to the docs (for now only the next version is provided), so we can update this in a followup PR.

(as a nice side-effect we can remove the # pylint: disable=line-too-long too, probably :D)

Add docs cross-linking to APIs #571

llucax · 2023-08-08T07:45:19Z

src/frequenz/sdk/timeseries/battery_pool/_result_types.py

+    This is the range within which power requests are NOT allowed by the battery pool.
+    If present, they will be a subset of the inclusion bounds.
+
+    More details [here](https://github.com/frequenz-floss/frequenz-api-common/blob/v0.3.0/proto/frequenz/api/common/metrics.proto#L37-L91).


tests/timeseries/_battery_pool/test_battery_pool.py

Signed-off-by: Leandro Lucarella <luca@llucax.com>

daniel-zullo-frequenz

Luca's suggestions about documentation sound good to me.
I can only add that the release notes should include a reference or link to the documentation of inclusion/exclusion bounds, otherwise the user needs to go through the code to get the info/links. But that can be also done in a separate PR

src/frequenz/sdk/timeseries/battery_pool/_metric_calculator.py

llucax · 2023-08-08T10:13:50Z

I can only add that the release notes should include a reference or link to the documentation of inclusion/exclusion bounds, otherwise the user needs to go through the code to get the info/links. But that can be also done is a separate PR

Lets do that in a separate commit, when also improving the links in the code.

To be merged after #537 Closes #152

shsms requested a review from a team as a code owner July 26, 2023 13:23

shsms requested a review from Marenz July 26, 2023 13:23

shsms marked this pull request as draft July 26, 2023 13:24

shsms force-pushed the battery-pool-excl-bounds branch 5 times, most recently from 782ec3f to 4a49683 Compare August 1, 2023 13:38

llucax added this to the v0.24.0 milestone Aug 2, 2023

llucax linked an issue Aug 2, 2023 that may be closed by this pull request

PowerDistributingActor should use min SoP (inclusion/exclusion bounds) #152

Closed

llucax removed a link to an issue Aug 2, 2023

PowerDistributingActor should use min SoP (inclusion/exclusion bounds) #152

Closed

shsms force-pushed the battery-pool-excl-bounds branch 2 times, most recently from e7e3066 to eb6e988 Compare August 3, 2023 12:12

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

shsms mentioned this pull request Aug 3, 2023

Support exclusion bounds in power distributor #562

Merged

shsms force-pushed the battery-pool-excl-bounds branch from eb6e988 to edf865d Compare August 4, 2023 15:40

shsms marked this pull request as ready for review August 4, 2023 15:42

shsms added 7 commits August 4, 2023 18:45

Rename internal inclusion-bound variables to latest naming scheme

5ef4d13

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Support fetching of exclusion bounds from the data sourcing actor

f4e51f2

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Use Power objects to represent power bounds in battery pool

bbf2a0c

They were represented with `float`s before. Closes frequenz-floss#524 Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Refactor inclusion_bounds extraction code to a separate method

dfa6edf

This is because once we add exclusion bounds, the function will become too big. And instead, we can add it as a separate method in a subsequent commit. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Implement fetching and streaming of exclusion bounds in battery pool

a3797b6

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

Update RELEASE_NOTES.md

5bb5bb3

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>

shsms force-pushed the battery-pool-excl-bounds branch from edf865d to 5bb5bb3 Compare August 4, 2023 16:45

github-actions bot removed part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:power-management Affects the management of battery power and distribution labels Aug 4, 2023

llucax previously approved these changes Aug 8, 2023

View reviewed changes

Replace Power.from_watts(0) with Power.zero()

639394c

Signed-off-by: Leandro Lucarella <luca@llucax.com>

llucax dismissed their stale review via 639394c August 8, 2023 09:28

llucax requested a review from daniel-zullo-frequenz August 8, 2023 09:28

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

View reviewed changes

src/frequenz/sdk/timeseries/battery_pool/_metric_calculator.py Outdated Show resolved Hide resolved

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

llucax assigned shsms Aug 8, 2023

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

shsms deleted the battery-pool-excl-bounds branch August 8, 2023 10:28

github-merge-queue bot pushed a commit that referenced this pull request Aug 9, 2023

Support exclusion bounds in power distributor (#562)

6258c76

To be merged after #537 Closes #152

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stream exclusion bounds from the battery pool #537

Stream exclusion bounds from the battery pool #537

Uh oh!

shsms commented Jul 26, 2023

Uh oh!

shsms commented Aug 4, 2023

Uh oh!

llucax left a comment

Uh oh!

llucax Aug 8, 2023

Uh oh!

llucax Aug 9, 2023

Uh oh!

llucax Aug 8, 2023

Uh oh!

Uh oh!

daniel-zullo-frequenz left a comment •

edited

Loading

Uh oh!

Uh oh!

llucax commented Aug 8, 2023

Uh oh!

Uh oh!

	More details [here](https://github.com/frequenz-floss/frequenz-api-common/blob/v0.3.0/proto/frequenz/api/common/metrics.proto#L37-L91).
	See [`frequenz.api.common.metrics_pb2.Metric.system_exclusion_bounds`][] for more details.

Stream exclusion bounds from the battery pool #537

Stream exclusion bounds from the battery pool #537

Uh oh!

Conversation

shsms commented Jul 26, 2023

Uh oh!

shsms commented Aug 4, 2023

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

llucax Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

llucax Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

llucax Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daniel-zullo-frequenz left a comment • 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!

Uh oh!

llucax commented Aug 8, 2023

Uh oh!

Uh oh!

daniel-zullo-frequenz left a comment •

edited

Loading