-
Notifications
You must be signed in to change notification settings - Fork 2
Add helper function to calculate energy metric #7
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 helper function to calculate energy metric #7
Conversation
m1.value * (m2.timestamp - m1.timestamp).total_seconds() | ||
for m1, m2 in zip(metric_samples, metric_samples[1:]) | ||
if m1.value > 0 | ||
) |
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 realize we are missing on the last value of the list. But we don't know the sampling period between the last sample in the list and the one that would follow.
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 discussed: For the last value we know the sampling period from the end time, i.e. metric_samples[-1].value * (end_time - metric_samples[-1].timestamp)
should be added to the sum.
src/frequenz/reporting/_reporting.py
Outdated
consumption = sum( | ||
m2.value - m1.value | ||
for m1, m2 in zip(metric_samples, metric_samples[1:]) | ||
if m1.value > 0 |
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 m2.value - m1.value > 0
(similar below).
end_time=end_time, | ||
consumption=consumption, | ||
production=production, | ||
) |
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 test cases for this function. For that you could mock the list...
method of the client.
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.
As discussed, test cases will be added in a follow up PR. I have added Issue #8.
b20f916
to
2878658
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 comment about the sign, otherwise LGTM
src/frequenz/reporting/_reporting.py
Outdated
If None, no resampling is applied. | ||
Returns: | ||
EnergyMetric: A named tuple with start_time, end_time, consumption, and production | ||
in Wh. |
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 should mention what sign production and consumption have.
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's included :-)
Signed-off-by: Flora <[email protected]>
2878658
to
910159f
Compare
Cumulative energy calculation with all its comments from this PR in new repo.