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

Flatline fix for Sensor Cards #2064

Merged
merged 6 commits into from
Nov 19, 2018
Merged

Flatline fix for Sensor Cards #2064

merged 6 commits into from
Nov 19, 2018

Conversation

Petro31
Copy link
Contributor

@Petro31 Petro31 commented Nov 17, 2018

This fixes issues where the first history item was repeated many times at the start of the graph resulting in a flat line that does not represent the data.

Also, the graph now has the ability to reach a 1 to 1 history graph to sensor graph point representation. Before 2 to 1 was the highest resolution possible. This was due to using history.length - 1 as the denominator in cases where the user set the accuracy larger than the total number of points in the history.

This fixes issues where the first history item was repeated many times at the start of the graph resulting in a flat line that does not represent the data.

Also, the graph now has the ability to reach a 1 to 1 history graph to sensor graph point representation.  Before 2 to 1 was the highest resolution possible.  This was due to using history.length - 1 as the denominator in cases where the user set the accuracy larger than the total number of points in the history.
@Petro31 Petro31 changed the title Update hui-sensor-card.js Flatline fix for Sensor Cards Nov 17, 2018
@balloob
Copy link
Member

balloob commented Nov 18, 2018

@kalkih , does this make sense?

@kalkih
Copy link
Contributor

kalkih commented Nov 18, 2018

Yes, this should solve the issue when there are fewer history entries than the accuracy set. 👍

There are more issues with the current calculation of the line though, the data points are just spread out over the x-axis and doesn't take the actual time of the history entry into account. Works fairly well for sensors that update regularly but not for sensors that updates sporadically.

I pretty much did a rewrite of the algorithm which now calculates the data points based on time and plots them out accordingly. Would have to replace the accuracy option with something like detail: 1 or 2, 1 for one point every hour, 2 for one point every 10 minute.

Edit: The new implementation would also assign the data points with a moving average for that hour/ ten minutes, instead of just the value of a single history entry, which should result in a better representation of the actual history.

Let me know if you want me to open a PR for this.

@balloob
Copy link
Member

balloob commented Nov 19, 2018

I would love for a PR that makes it better 👍 would that supersede this PR ?

@Petro31
Copy link
Contributor Author

Petro31 commented Nov 19, 2018

@balloob, @kalkih I can do the other PR that fixes the algorithm. But as you can probably tell from my commits, I don't have a dev environment. I made the changes in the custom card and placed them here. I got a new system coming in the mail today and I'll have a dev environment setup within the next week. When that happens, I can make a better PR if you guys do not have the time.

I didn't want to make major changes without being able to fully test inside the suite. This PR was encapsulated in 1 function that had the same variables names. I was confident this would work no matter what.

@balloob
Copy link
Member

balloob commented Nov 19, 2018

Will merge this and @kalkih can open a better PR when ready.

@balloob balloob merged commit 9a9986c into home-assistant:dev Nov 19, 2018
@ghost ghost removed the in progress label Nov 19, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants