-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add assets config to microgrid config #17
Conversation
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.
Nice work on adding support for asset configurations and integrating _assets_cfg
into MicrogridConfig
is a solid addition. I can see how this will make the configuration more modular and extendable in the future. I only have a minor comment/suggestion.
src/frequenz/lib/notebooks/config.py
Outdated
self._assets_cfg = AssetsConfig( | ||
pv=config_dict.get("pv") or {}, | ||
) |
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.
Is there a specific reason for only including pv
for now?
I suggest updating the __init__
method to initialize _assets_cfg
with configurations for all three asset types. This would make the behaviour consistent with the AssetsConfig
design.
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.
Good point, will update
Config files specifying assets pv, wind and battery are now supported and asset metadata can be accessed via the `assets` property. For each asset type multiple entries can be specified in dict form. Signed-off-by: cwasicki <[email protected]>
Updated. |
Config files specifying assets pv, wind and battery are now supported and asset metadata can be accessed via the
assets
property. For each asset type multiple entries can be specified in dict form.