-
Notifications
You must be signed in to change notification settings - Fork 17
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 pv optimization tutorial #700
base: v1.x.x
Are you sure you want to change the base?
Add pv optimization tutorial #700
Conversation
eabafd2
to
9c44f4a
Compare
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.
Just a few early comments.
docs/tutorials/pv_optimization.md
Outdated
In this tutorial we want to write an application that optimizes the energy produced from a PV system with | ||
Battery for self consumption. | ||
In order to do so we need to measure the power that flows through the grid connection point | ||
(TODO link to glossary) to determine excess power. |
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.
You can link also PV and other terms, I guess we should only link the first time a term appears so we don't clutter the docs too much.
docs/tutorials/pv_optimization.md
Outdated
and consumed power is positive. | ||
|
||
We want to measure the excess power. In order to do so you can use the SDK's data pipeline and especially | ||
the pre defined consumer and producer power formulas (TODO is there a documentation for those?) |
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.
There should be at least in the code, you can link to code using [link title][full.public.path.to.python.symbol]
, like [consumer][frequenz.sdk.timeseries.logical_meter.LogicalMeter.consumer_power]
.
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.
Does anything speak against using relative links? (../../timeseries/...)
|
||
1. The initialization code as explained in the Getting Started tutorial. | ||
2. Construct the consumer excess power by summing up consumer and producer power each of which having | ||
opposite signs due to the sign convention. This returns a formula engine. |
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.
It would be nice to link to FormulaEngine
here, but, it is not public :-/
I think all classes that are returned should be public, because otherwise the user doesn't know what they have to offer, even when looking at the API docs, for example:
(no link in the return type, so the user don't know what can be done with it)
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.
Yeah this is silly
9c44f4a
to
40c5083
Compare
Signed-off-by: Matthias Wende <[email protected]>
40c5083
to
1f43f35
Compare
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.
Content lgtm, just got comments about the links and number comments in the last example.
In this tutorial you will write an application that optimizes the energy produced from a | ||
[PV](intro/glossary/#pv) system with Battery for self consumption. | ||
In order to do so you need to measure the power that flows through the | ||
[grid connection point](../../intro/glossary/#grid) to determine excess power. |
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 #722 is merged, we'll no longer have to type paths and slugs directly:
[grid connection point](../../intro/glossary/#grid) to determine excess power. | |
{{glossary("Grid", "grid connection point")}} to determine excess power. |
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.
also the dir is not called "intro" anymore, it got renamed to "user-guide".
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.
#722 is merged btw, so this is available upstream.
|
||
We want to measure the excess power. In order to do so you can use the SDK's data pipeline and especially | ||
the pre defined | ||
[consumer](../../reference/frequenz/sdk/timeseries/logical_meter/#frequenz.sdk.timeseries.logical_meter.LogicalMeter.consumer_power) |
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.
This should be
[consumer](../../reference/frequenz/sdk/timeseries/logical_meter/#frequenz.sdk.timeseries.logical_meter.LogicalMeter.consumer_power) | |
[consumer][frequenz.sdk.timeseries.logical_meter.LogicalMeter.consumer_power] |
).build("excess_power") # (2)! | ||
cons_excess_power_recv = consumer_excess_power_engine.new_receiver() # (3)! | ||
|
||
battery_pool = microgrid.battery_pool() # (1)! |
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.
do the numbered comments make sense when there are no points below the block? does it reuse the numbered comments from previous examples?
continue | ||
if cons_excess_power <= Power.zero(): # (5)! | ||
await battery_pool.charge(-cons_excess_power) | ||
elif cons_excess_power > Power.zero(): |
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.
Nitpick: the condition here is redundant, it can just be a simple else
, but maybe you want to make it explicit for educational purposes.
You could also use propose_power()
, right? I guess you are not using it also for educational purposes?
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.
oh yes, the charge
/discharge
methods are no longer available, need to be replaced by the new propose_*
ones.
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.
We should ideally enable examples testing for docs/
too. @Marenz do you think it would be simple to implement?
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 believe it isn't hard, but it will probably need some reading up on how exactly do do it, maybe half a days work?
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 think it might be worth it now that we have more examples in docs/
.
I would also like to resurrect the idea about just extracting examples to separate files and run all the tools as normal on them. Another advantage of this is we need to run pytest
in all combinations of python and archs, but linting could be done only once, there is no need to run the linting on all python versions and archs, and in particular testing examples is super slow, so it makes test in arm64 take much longer than needed.
But the later is probably a fair amount of extra work, so we can also do it as a second step and first only focus on getting docs/
tested.
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.
|
||
## Measure the excess power | ||
|
||
When using the term excess power what we actually mean is the consumer excess power, that is the power that |
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.
quotes would be good around the term here
No description provided.