Skip to content
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 delta option in aggregate_func #329

Merged
merged 3 commits into from
Apr 15, 2020
Merged

Add delta option in aggregate_func #329

merged 3 commits into from
Apr 15, 2020

Conversation

PierreB49
Copy link
Contributor

This option allows to show delta for instance to show daily or hourly consumption from cumulative data.

This option allows to show delta for instance to show daily or hourly consumption from cumulative data.
Copy link
Contributor

@maxwroc maxwroc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, before addressing my comments please wait for @kalkih opinion.

Nice and simple extension please update documentation and give the example use case there (maybe update/mention this in the energy usage example)

edit:
BTW I don't know how come I didn't came up with this solution (my first change for this lib was actually to give a way to show the energy daily consumption). I think I was too focused on the getting the results form utility meter (not direct results from the sensor) :) Good job!

src/graph.js Outdated Show resolved Hide resolved
src/graph.js Outdated Show resolved Hide resolved
src/graph.js Outdated Show resolved Hide resolved
@PierreB49
Copy link
Contributor Author

Hey, before addressing my comments please wait for @kalkih opinion.

Nice and simple extension please update documentation and give the example use case there (maybe update/mention this in the energy usage example)

edit:
BTW I don't know how come I didn't came up with this solution (my first change for this lib was actually to give a way to show the energy daily consumption). I think I was too focused on the getting the results form utility meter (not direct results from the sensor) :) Good job!

Thanks you for your reply. I'm a beginner in nodejs so there might be room for improvement in my code. I adapted the card for my usage and thought it could be useful for someone else.

@maxwroc
Copy link
Contributor

maxwroc commented Apr 12, 2020

Hey, before addressing my comments please wait for @kalkih opinion.
Nice and simple extension please update documentation and give the example use case there (maybe update/mention this in the energy usage example)
edit:
BTW I don't know how come I didn't came up with this solution (my first change for this lib was actually to give a way to show the energy daily consumption). I think I was too focused on the getting the results form utility meter (not direct results from the sensor) :) Good job!

Thanks you for your reply. I'm a beginner in nodejs so there might be room for improvement in my code. I adapted the card for my usage and thought it could be useful for someone else.

And it is awesome that you have decided to share the solution and update the lib!

One thing worth to mention additionally in the documentation is that it may not give exactly correct results. What I mean here is that in theory we should store somewhere the last state from previous time bucket and calc delta to the value from the next one. Here you calculate delta within single time bucket which in some corner case situations may produce incorrect results.
But anyway I think for majority of people this won't be a problem so I'm fine with this approach as the implementation is simple and does the job. It will be useful for people who don't want to add another sensor - utility meter.

I wonder what is @kalkih's optionion. I remember he didn't want to add similar functionality as it can be achieved via utility meter.

@kalkih kalkih added the feature request New feature or request label Apr 13, 2020
@kalkih
Copy link
Owner

kalkih commented Apr 13, 2020

Hello, this is indeed a nice addition to the card, thanks!
As Max said I've pointed people to utility meter when they've asked for functionality like this but I'm ok with this solution it's simple & could work fairly well in some configurations.

Just address the comments made by Max and add it to the documentation and I'll merge this, thank you!

this.coords = this._calcPoints(coords);
this.min = Math.min(...this.coords.map(item => Number(item[V])));
this.max = Math.max(...this.coords.map(item => Number(item[V])));
this.histGroups = this._calcPoints(histGroups);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have edited little bit too much now ;) The local var name which I have asked to change was confusing because it wasn't containing graph coords and the name was exactly the same as the class property (so the impression was that the type of the value was the same - which was not true).
Class property this.histGroups doesn't contain grouped history values (as name suggests) but actually calculated graph points/coords. Please leave here the old name.

@maxwroc
Copy link
Contributor

maxwroc commented Apr 15, 2020

This probably covers requests: #197, #288

@maxwroc
Copy link
Contributor

maxwroc commented Apr 15, 2020

@kalkih can you merge this please? my last comment is a minor thing which we can do in another PR

@kalkih kalkih merged commit 9bd7474 into kalkih:dev Apr 15, 2020
@kalkih
Copy link
Owner

kalkih commented Apr 15, 2020

Great job, thanks for the contribution!

Needs to be added to the docs before next release.

@maxwroc
Copy link
Contributor

maxwroc commented Apr 15, 2020

Ok. Let me do it

@maxwroc
Copy link
Contributor

maxwroc commented Apr 15, 2020

#332

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants