Implement the `ShiftingMatryoshka` algorithm for the PowerManager #1146

shsms · 2025-01-15T14:58:26Z

This PR removes the set_operating_point feature and replaces the power manager's original Matryoshka algorithm with the new ShiftingMatryoshka algorithm.

With the new algorithm, power proposals from actors are added to get the target power for the components, and higher-priority actors can limit the bounds available to lower-priority actors.

llucax

I just gave this a quick look, only to the structural changes.

I wonder why removing the old algorithm if we can support more than one. I can understand if it is to remove the maintenance cost, but I would delay the removal until we collected more experience.

src/frequenz/sdk/microgrid/_power_managing/_power_managing_actor.py

src/frequenz/sdk/microgrid/__init__.py

shsms · 2025-01-20T13:31:08Z

I wonder why removing the old algorithm if we can support more than one. I can understand if it is to remove the maintenance cost, but I would delay the removal until we collected more experience.

This was because pylint was complaining about duplicate lines, plus it is a separate commit, which should be easy to revert if necessary. If you prefer, I can drop that commit, but we'll need to find a way to silence pylint either by refactoring the tests or pylint config.

But if we remove the current PR's limiting-only option like I mentioned in some comments above, this would stop being an issue as well, so we can revisit this after we decide that.

llucax · 2025-01-22T08:17:45Z

This was because pylint was complaining about duplicate lines, plus it is a separate commit, which should be easy to revert if necessary. If you prefer, I can drop that commit, but we'll need to find a way to silence pylint either by refactoring the tests or pylint config.

We should definitely not remove code because pylint complains about duplication :D

Can't we just add a # pylint: disable=<duplication> at the beginning of the file(s) reporting duplication?

…requenz-floss#970)" This reverts commit 6a54a0b, reversing changes made to 4c71140. Signed-off-by: Sahas Subramanian <[email protected]>

…er (frequenz-floss#957)" This reverts commit d5d74a3, reversing changes made to 8e5d65e. Signed-off-by: Sahas Subramanian <[email protected]>

This makes the function easier to distinguish from per-priority bounds produced by the power manager. Signed-off-by: Sahas Subramanian <[email protected]>

The shifting logic will be implemented in the next commit. Signed-off-by: Sahas Subramanian <[email protected]>

shsms · 2025-03-21T16:47:23Z

This is ready for review again.

Copilot

Pull Request Overview

This PR replaces the original Matryoshka algorithm with the new ShiftingMatryoshka algorithm in the power manager while removing the “set_operating_point” parameter from pool APIs and associated documentation.

Introduces a new algorithm implementation in _shifting_matryoshka.py that aggregates proposals using shifted bounds.
Updates documentation, tests, and pool creation methods to reflect the removal of the set_operating_point parameter.
Refactors the power managing actor to use a unified subscriptions structure and maps the algorithm selection accordingly.

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/frequenz/sdk/microgrid/_power_managing/_shifting_matryoshka.py	New algorithm implementation and handling of bounds via power shifting.
src/frequenz/sdk/microgrid/init.py	Updated documentation to describe the new handling of power proposals.
src/frequenz/sdk/timeseries/*	Removed obsolete set_operating_point parameter and cleaned up related docstrings.
tests/*	Updated tests to match changes in target power calculation and bounds shifting.
RELEASE_NOTES.md	Updated to document removal of the set_operating_point parameter and details of the new algorithm.
src/frequenz/sdk/microgrid/_power_managing/_power_managing_actor.py	Refactored to use the new algorithm selection and unified subscriptions mapping.
src/frequenz/sdk/microgrid/_data_pipeline.py	Removed extra set_operating_point parameter from pool creation methods.

Comments suppressed due to low confidence (2)

src/frequenz/sdk/microgrid/_power_managing/_shifting_matryoshka.py:147

[nitpick] Verify that subtracting the proposal power from the lower bound produces the intended result in all edge cases. Consider adding an inline comment to explain the rationale behind shifting the bounds in this manner.

lower_bound = lower_bound - proposal_power

src/frequenz/sdk/microgrid/_power_managing/_power_managing_actor.py:79

[nitpick] Consider renaming '_subscriptions' to a more descriptive name (e.g. '_report_subscriptions') to clarify that these senders are used for reporting status.

self._subscriptions: dict[frozenset[int], dict[int, Sender[_Report]]] = {}

llucax

Didn't do a super in-depth review, in particular I didn't try to validate or fully understand the algorithm code, but I did follow the examples/new tests to understand the algorithm and it looks good to me and it was easier to follow (not sure if because I'm now more familiar with the algorithm or because the new wording focused on shifting :D).

Just a few minor comments, otherwise LGTM.

One final question though. The old matryoshka algorithm still works as before the operating point setting/group/shifting was added, right? The removal of the set_operating_point option only affects that?

llucax · 2025-03-24T08:35:42Z

src/frequenz/sdk/microgrid/_power_managing/_shifting_matryoshka.py

+"""A power manager implementation that uses the matryoshka algorithm.
+
+When there are multiple proposals from different actors for the same set of components,
+the matryoshka algorithm will consider the priority of the actors, the bounds they set
+and their preferred power to determine the target power for the components.
+
+The preferred power of lower priority actors will take precedence as long as they
+respect the bounds set by higher priority actors.  If lower priority actors request
+power values outside the bounds set by higher priority actors, the target power will
+be the closest value to the preferred power that is within the bounds.
+
+When there is only a single proposal for a set of components, its preferred power would
+be the target power, as long as it falls within the system power bounds for the
+components.


I would move this docs to the class, as the module is private, it will never be rendered in the documentation website otherwise (unless you are explicitly including it in docs/.

oh wow, I forgot to update these docs. These are for the old matryoshka algorithm. I will do that and move it to the class.

Hehe, right, I was reviewing commit by commit and commented on this when I saw you added the file but then didn't realized you didn't update it afterwards.

llucax · 2025-03-24T08:39:46Z

src/frequenz/sdk/microgrid/_power_managing/_shifting_matryoshka.py

+        proposals: set[Proposal],
+        system_bounds: SystemBounds,
+        priority: int | None = None,
+    ) -> tuple[Power | None, Bounds[Power]]:


I'm shaking while about bounds due to PTSD after fighting with bounds updates for the v0.17 microgrid API. I hope it is not too hard to merge 😱

it should be fine because you're handling it in the compatibility layer from the client right?

llucax · 2025-03-24T12:07:22Z

src/frequenz/sdk/microgrid/_power_managing/_shifting_matryoshka.py

+            proposal_lower = next_proposal.bounds.lower or lower_bound
+            proposal_upper = next_proposal.bounds.upper or upper_bound
+            proposal_power = next_proposal.preferred_power
+
+            if proposal_upper < proposal_lower:
+                continue
+
+            if proposal_power and (
+                proposal_power < proposal_lower or proposal_power > proposal_upper
+            ):
+                continue
+
+            if proposal_lower >= upper_bound:
+                proposal_lower = upper_bound
+                proposal_upper = upper_bound
+            elif proposal_upper <= lower_bound:
+                proposal_lower = lower_bound
+                proposal_upper = lower_bound
+
+            lower_bound = max(lower_bound, proposal_lower)
+            upper_bound = min(upper_bound, proposal_upper)


Are all these checks for proposal_power, proposal_lower and proposal_upper intentionally covering for an eventual 0.0 besides None or was the intention to only match None? Since in the code below, you are actually checking for None explicitly, I guess it is intentional, but maybe it would be best to add some comment to say it explicitly (either way).

intentionally covering for an eventual 0.0 besides None or was the intention to only match None

I don't understand what that means. Of course, I can add comments for these, though.

added more comments, also fixed a bug.

llucax · 2025-03-24T12:17:23Z

src/frequenz/sdk/microgrid/__init__.py

+| Actor | Priority | System Bounds   | Requested Bounds | Requested    | Aggregate |
+|       |          |                 |                  | Power        | Power     |
+|-------|----------|-----------------|------------------|--------------|-----------|
+| A     | 3        | -100kW .. 100kW | None             | 20kW         | 20kW      |
+| B     | 2        | -120kW .. 80kW  | None             | 50kW         | 70kW      |
+| C     | 1        | -170kW .. 30kW  | None             | 50kW         | 100kW     |
+|       |          |                 |                  | target power | 100kW     |


Maybe like in the test, adding a column with the "adjusted power" could add some extra clarity.

llucax · 2025-03-24T12:33:08Z

src/frequenz/sdk/microgrid/__init__.py

@@ -174,53 +174,128 @@
 controlling batteries, power could be distributed based on the `SoC` of the
 individual batteries, to keep the batteries in balance.

-### Resolving conflicting power proposals
+### How to work with other actors


All (or at least some of) the old documentation here should still be useful for people interested in the old Matryoshka algorithm, so not sure if replacing it is the best choice. I wonder if the docs shouldn't be moved to the algorithms themselves, and either referenced here (or see if there is a way to include it with mkdocs, but this won't work/look nice when reading the docs from the editor). Maybe in here we can just say more generically that there are 2 available algorithms, say which is the default, and then just link to the algorithm docs for details? If the plan is to remove the old algo soon, I guess it is fine to move forward as it is, but if we really want to support more than one algorithm, the docs should be structured in a way that we can explain all of them and switch the default easily without rewriting the whole __init__.py docs.

The code for the old algorithm is still there, but users cannot choose it. Chances are we'll not need the old algorithm anymore. We could introduce new algorithms in the future just for specific component types, like EVs, etc. But we don't have to change the docs structure until we offer multiple algorithms for them to choose from.

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

With this we only have to write the details on how the power manager works only once. Signed-off-by: Sahas Subramanian <[email protected]>

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

Earlier, the exclusion bounds were applied to every proposal, and in some cases, that was leading to much higher target powers than what was necessary. Signed-off-by: Sahas Subramanian <[email protected]>

And refactor the _calc_targets method to use the new function to eliminate code duplication. Signed-off-by: Sahas Subramanian <[email protected]>

Instead the proposed powers need to be clamped to the available bounds. Signed-off-by: Sahas Subramanian <[email protected]>

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

shsms requested a review from a team as a code owner January 15, 2025 14:58

shsms requested review from daniel-zullo-frequenz and removed request for a team January 15, 2025 14:58

github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline part:microgrid Affects the interactions with the microgrid labels Jan 15, 2025

shsms force-pushed the power-manager-shift-limit branch from 649caf6 to 3c38ef5 Compare January 15, 2025 14:59

shsms requested a review from llucax January 15, 2025 15:08

shsms linked an issue Jan 15, 2025 that may be closed by this pull request

PowerManager algorithm that can shift or limit bounds #1125

Open

shsms self-assigned this Jan 15, 2025

llucax reviewed Jan 16, 2025

View reviewed changes

src/frequenz/sdk/microgrid/_power_managing/_power_managing_actor.py Show resolved Hide resolved

src/frequenz/sdk/microgrid/_power_managing/_power_managing_actor.py Outdated Show resolved Hide resolved

llucax reviewed Jan 16, 2025

View reviewed changes

src/frequenz/sdk/microgrid/__init__.py Outdated Show resolved Hide resolved

src/frequenz/sdk/microgrid/__init__.py Outdated Show resolved Hide resolved

llucax reviewed Jan 16, 2025

View reviewed changes

src/frequenz/sdk/microgrid/__init__.py Outdated Show resolved Hide resolved

shsms force-pushed the power-manager-shift-limit branch from 3c38ef5 to 7f405bb Compare January 20, 2025 13:00

shsms mentioned this pull request Jan 24, 2025

Drop empty Matryoshka buckets #1155

Merged

shsms force-pushed the power-manager-shift-limit branch from 7f405bb to 509c528 Compare January 28, 2025 10:39

shsms force-pushed the power-manager-shift-limit branch 6 times, most recently from eff0cf3 to 6e13739 Compare March 20, 2025 16:49

shsms added 4 commits March 21, 2025 10:09

Revert "Rename parameter in_shifting_group → set_operating_point (f…

4dd9c15

…requenz-floss#970)" This reverts commit 6a54a0b, reversing changes made to 4c71140. Signed-off-by: Sahas Subramanian <[email protected]>

Revert "Support power requests from shifting actors in the PowerManag…

27fc177

…er (frequenz-floss#957)" This reverts commit d5d74a3, reversing changes made to 8e5d65e. Signed-off-by: Sahas Subramanian <[email protected]>

Rename _add_bounds_tracker → _add_system_bounds_tracker

edd4fd8

This makes the function easier to distinguish from per-priority bounds produced by the power manager. Signed-off-by: Sahas Subramanian <[email protected]>

Clone the Matryoshka implementation and name it ShiftingMatryoshka

8d45fc9

The shifting logic will be implemented in the next commit. Signed-off-by: Sahas Subramanian <[email protected]>

shsms force-pushed the power-manager-shift-limit branch from 6e13739 to 4b858b9 Compare March 21, 2025 09:10

shsms force-pushed the power-manager-shift-limit branch from fdf5479 to 68c72cd Compare March 21, 2025 16:48

llucax requested a review from Copilot March 24, 2025 08:22

Copilot AI reviewed Mar 24, 2025

View reviewed changes

llucax self-requested a review March 24, 2025 08:25

llucax reviewed Mar 24, 2025

View reviewed changes

shsms added 9 commits March 27, 2025 10:14

Implement the adding of powers and shifting/limiting of bounds

93236a3

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

Unittest the shifting matryoshka algorithm

34814cf

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

Integrate new ShiftingMatryoshka algorithm into the PowerManager

e9271ac

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

Make ShiftingMatryoshka the default power manager algorithm

b43c45c

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

Update microgrid documentation with new power manager behaviour

f9b884b

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

Update propose_power docs to refer to microgrid docs

89978e0

With this we only have to write the details on how the power manager works only once. Signed-off-by: Sahas Subramanian <[email protected]>

Update release notes

41e0a3a

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

Apply exclusion bounds only after the target power is calculated

5caf4ae

Earlier, the exclusion bounds were applied to every proposal, and in some cases, that was leading to much higher target powers than what was necessary. Signed-off-by: Sahas Subramanian <[email protected]>

Add _get_nearest_possible_power function

07b1c1b

And refactor the _calc_targets method to use the new function to eliminate code duplication. Signed-off-by: Sahas Subramanian <[email protected]>

shsms force-pushed the power-manager-shift-limit branch from 68c72cd to 9cb09d7 Compare March 27, 2025 10:01

shsms requested a review from llucax March 27, 2025 10:03

shsms added 2 commits March 27, 2025 11:04

Don't bypass proposals with out-of-bounds power requests

cafa563

Instead the proposed powers need to be clamped to the available bounds. Signed-off-by: Sahas Subramanian <[email protected]>

Add code comments explaining the ShiftingMatryoshka implementation

95e326c

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

shsms force-pushed the power-manager-shift-limit branch from 9cb09d7 to 95e326c Compare March 27, 2025 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the `ShiftingMatryoshka` algorithm for the PowerManager #1146

Implement the `ShiftingMatryoshka` algorithm for the PowerManager #1146

shsms commented Jan 15, 2025 •

edited

Loading

llucax left a comment

shsms commented Jan 20, 2025

llucax commented Jan 22, 2025

shsms commented Mar 21, 2025

Copilot AI left a comment

llucax left a comment

llucax Mar 24, 2025

shsms Mar 24, 2025

llucax Mar 24, 2025

shsms Mar 27, 2025

llucax Mar 24, 2025

shsms Mar 27, 2025

llucax Mar 24, 2025

shsms Mar 26, 2025

shsms Mar 27, 2025

llucax Mar 24, 2025

shsms Mar 27, 2025

llucax Mar 24, 2025

shsms Mar 26, 2025

Implement the ShiftingMatryoshka algorithm for the PowerManager #1146

Are you sure you want to change the base?

Implement the ShiftingMatryoshka algorithm for the PowerManager #1146

Conversation

shsms commented Jan 15, 2025 • edited Loading

llucax left a comment

Choose a reason for hiding this comment

shsms commented Jan 20, 2025

llucax commented Jan 22, 2025

shsms commented Mar 21, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

llucax left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement the `ShiftingMatryoshka` algorithm for the PowerManager #1146

Implement the `ShiftingMatryoshka` algorithm for the PowerManager #1146

shsms commented Jan 15, 2025 •

edited

Loading