- 
                Notifications
    You must be signed in to change notification settings 
- Fork 20
          Add a PVPool implementation
          #914
        
          New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6cb9030    to
    7f4e115      
    Compare
  
    | CI is failing, I'm starting a review anyway, so please unless it is very small and trivial stuff (or adding more commits to amend later), please don't update the PR. | 
| Added a few commits including tests, docu updates, release notes and a fixup for a bugfix. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again, very nice job and it is really nice to see al this finally coming together! 💯
        
          
                src/frequenz/sdk/actor/_power_managing/_power_managing_actor.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/frequenz/sdk/actor/_power_managing/_power_managing_actor.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...dk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py
          
            Show resolved
            Hide resolved
        
              
          
                src/frequenz/sdk/timeseries/pv_pool/_pv_pool_reference_store.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | I've squashed the old fixups, will address the comments in new fixups. | 
| I will squash the commits around 3pm. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will approve to speed up things, but I'm still concerned about #914 (comment), if we are not logging unexpected errors in other pools too, we can create a separate issue to fix this, but I think silently swallowing exceptions should never happen.
625378b    to
    faecdbf      
    Compare
  
            
          
                src/frequenz/sdk/actor/power_distributing/_component_managers/_battery_manager.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...dk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                .../sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_ev_charger_manager.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                .../sdk/actor/power_distributing/_component_managers/_ev_charger_manager/_ev_charger_manager.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...dk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...dk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...dk/actor/power_distributing/_component_managers/_pv_inverter_manager/_pv_inverter_manager.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/frequenz/sdk/actor/power_distributing/_component_managers/_battery_manager.py
              
                Outdated
          
            Show resolved
            Hide resolved
        
      Signed-off-by: Sahas Subramanian <[email protected]>
These system bounds trackers need to be moved out of the `*Pool`s, but exactly how, is yet to be decided. Until then, I'm following the same pattern here as in the other `*Pool`s. This commit adds a `_system_power_bounds` method only because that's all that's needed for the PowerManager to support PV inverters. Other methods will be added later. Signed-off-by: Sahas Subramanian <[email protected]>
There is a plan to rename these constructors to `microgrid.new_*_pool()`. Not doing that now, so that it can be done together for all the pools in a separate PR. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
This is done through the new `PVPool.propose_power` method. This commit also adds a `component_ids` property, that exposes the IDs of the PV inverters managed by this PVPool. Signed-off-by: Sahas Subramanian <[email protected]>
Also update the PV formula builder to use the component IDs from the PV pool if specified. 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]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
This affects the PowerManager, PowerDistributor and PowerWrapper. The `component_type` paramater now also has a default value of `None`. Signed-off-by: Sahas Subramanian <[email protected]>
Co-authored-by: Leandro Lucarella <[email protected]> Signed-off-by: Sahas Subramanian <[email protected]>
Co-authored-by: Leandro Lucarella <[email protected]> Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Also use `logging.exception` to log broad exceptions Signed-off-by: Sahas Subramanian <[email protected]>
| I added all the exception related fixes to the latest commit. | 
| Also squashed the commits. | 
| Enabling auto-merge | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 🎉
No description provided.