-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
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
Keep track of the derivative for unit_time #31397
Keep track of the derivative for unit_time #31397
Conversation
Hey there @afaucogney, mind taking a look at this pull request as its been labeled with a integration ( |
b995aad
to
3b3a5da
Compare
unit_time is not about relevance, this is a part of the unit of the derivative result, that you can calculate for different time scale. |
I've also editted the body of the first post in this PR. |
bb3ce69
to
27f8936
Compare
Don't forget to add a test. |
4d20d5b
to
008f24f
Compare
In this way, you will get a better estimate of the derivate during the timescale that is relavant to the sensor. This solved a problem where sensors have a low output resolution. For example a temperature sensor that can only be integer numbers. It might report many values that are the same and then suddenly go up one value. Only in that moment (with the current implementation) the derivative will be finite. With my proposed implementation, this problem will not occur, because it takes the average derivative of the last `unit_time`.
a6fb736
to
243d75c
Compare
@balloob, I have added a test! 👍 I've also refactored the tests and found an actual bug (division instead of multiplication of There was a test that confirmed the broken state, which I've fixed in 3f9fcac. There was also a bug in the |
c94d87e
to
359e9c2
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.
Looks good to me, especially the tests look pretty clean now 👌🎉
* update the docs, see home-assistant/core#31397 * add more detailed example
@basnijholt, I'm happy you deeply reviewed the derivative integration. I'm neither a "python guy", neither a "derivative guy". However I still have some open question: => it looks better to me, and will provide less confusion. (with the first chart, it seems the original is almost always zero +- something)
=> From this fact, how the true_derivative can be higher (in value) than the calculated one (the original one) ? PS: As I said at the beginning of the "derivative" PR, this code is based on the "integration" integration. So maybe you should have a look on it, as you discovered many findings here ! |
* keep track of the derivative for unit_time In this way, you will get a better estimate of the derivate during the timescale that is relavant to the sensor. This solved a problem where sensors have a low output resolution. For example a temperature sensor that can only be integer numbers. It might report many values that are the same and then suddenly go up one value. Only in that moment (with the current implementation) the derivative will be finite. With my proposed implementation, this problem will not occur, because it takes the average derivative of the last `unit_time`. * only loop as much as needed * treat the special case of 1 entry * add option time_window * use cv.time_period * fix comment * set time_window=0 by default * rephrase comment * use timedelta for time_window * fix the "G" unit_prefix and add more prefixes https://en.wikipedia.org/wiki/Unit_prefix * add debugging lines * simplify logic * fix bug where the there was a division of unit_time instead of multiplication * simplify tests * add test_data_moving_average_for_discrete_sensor * fix test_dataSet6 * improve readability of the tests * better explain the test * remove debugging log lines
Hi @afaucogney, you are right! I made a mistake in my plot. I both used Also, as you have correctly done, in my plot I didn't divide by Δx. Your analysis shows, that if you have a densely sampled discrete sensor, that the derivate gives high peaks (which will become delta peaks in the limit of I am doing some more small checks now. Since now we essentially do a moving average, the derivative that we calculate is actually of |
Breaking change
The derivative component is still in beta.
Proposed change
With this implementation, we will get a better estimate of the derivate during the timescale that is relevant to the sensor (indicated by the new option
time_window
).This solves a problem where sensors have a low output resolution.
For example, a temperature sensor that can only report integer numbers (or half-integer).
It might report values every few seconds that are the same (e.g. 18 deg) and then suddenly go up one value (to 19 deg).
Only at that moment (with the current implementation) the derivative will be finite, otherwise, it will be zero. This instantaneous derivative isn't a good estimate of the true derivative.
The problem is captured by the following plot:
With my proposed implementation, this problem will not occur, because it takes the average derivative of
time_window
.I have added a
time_window
option, if you leave it out or if you set it to 0, the behavior is as it is in thedev
branch.Config example:
will average over the last half hour.
Type of change
Example entry for
configuration.yaml
:# Example configuration.yaml
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
Documentation added/updated for Derivative component update, add time_window home-assistant.io#11963
If the code communicates with devices, web services, or third-party tools:
The manifest file has all fields filled out correctly.
Updated and included derived files by running:
python3 -m script.hassfest
.New or updated dependencies have been added to
requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.Untested files have been added to
.coveragerc
.The integration reached or maintains the following Integration Quality Scale: