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

Keep track of the derivative for unit_time #31397

Merged
merged 19 commits into from
Feb 3, 2020

Conversation

basnijholt
Copy link
Contributor

@basnijholt basnijholt commented Feb 2, 2020

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:

import matplotlib.pyplot as plt
import numpy as np
xs = np.linspace(0, 2 * np.pi)
true_sensor = 4 * np.sin(xs)
sensor_output = np.round(true_sensor, 0)
plt.plot(xs, true_sensor, label='true sensor')
plt.plot(xs, sensor_output, label='sensor value')
plt.scatter(xs, sensor_output, c="C1")
plt.plot(xs, np.gradient(sensor_output), label='calculated derivative (with current method)')
plt.plot(xs, 5*np.cos(xs), label='true derivative')
plt.legend()

image

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 the dev branch.

Config example:

sensor:
  - platform: derivative
    source: sensor.gas_consumption
    name: Gas per hour
    unit: m³/h
    unit_time: h
    time_window: "00:30:00"

will average over the last half hour.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link

Hey there @afaucogney, mind taking a look at this pull request as its been labeled with a integration (derivative) you are listed as a codeowner for? Thanks!

@MartinHjelmare MartinHjelmare changed the title keep track of the derivative for unit_time Keep track of the derivative for unit_time Feb 2, 2020
@afaucogney
Copy link
Contributor

With this implementation, we will get a better estimate of the derivate during the timescale that is relevant to the sensor (indicated by unit_time).

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.

@basnijholt
Copy link
Contributor Author

basnijholt commented Feb 2, 2020

I've also editted the body of the first post in this PR.

@basnijholt basnijholt force-pushed the derivative-improvement branch from bb3ce69 to 27f8936 Compare February 2, 2020 21:59
@basnijholt basnijholt requested a review from springstan February 2, 2020 22:01
@balloob
Copy link
Member

balloob commented Feb 2, 2020

Don't forget to add a test.

@basnijholt basnijholt force-pushed the derivative-improvement branch 8 times, most recently from 4d20d5b to 008f24f Compare February 3, 2020 11:38
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`.
@basnijholt basnijholt force-pushed the derivative-improvement branch from a6fb736 to 243d75c Compare February 3, 2020 14:49
@basnijholt
Copy link
Contributor Author

basnijholt commented Feb 3, 2020

@balloob, I have added a test! 👍

I've also refactored the tests and found an actual bug (division instead of multiplication of unit_time) and fixed it (see this commit 243d75c).

There was a test that confirmed the broken state, which I've fixed in 3f9fcac.

There was also a bug in the UNIT_PREFIXES, where G was 1e6 instead of 1e9. In addition, I have added some more common prefixes.

@basnijholt basnijholt force-pushed the derivative-improvement branch from c94d87e to 359e9c2 Compare February 3, 2020 17:36
@basnijholt basnijholt requested a review from springstan February 3, 2020 17:38
Copy link
Member

@springstan springstan left a 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 👌🎉

basnijholt added a commit to basnijholt/home-assistant.io that referenced this pull request Feb 3, 2020
basnijholt added a commit to basnijholt/home-assistant.io that referenced this pull request Feb 3, 2020
@balloob balloob merged commit 119566f into home-assistant:dev Feb 3, 2020
frenck pushed a commit to home-assistant/home-assistant.io that referenced this pull request Feb 4, 2020
* update the docs, see home-assistant/core#31397

* add more detailed example
@afaucogney
Copy link
Contributor

afaucogney commented Feb 4, 2020

@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:
1/ First I update you plot with same proportional factor for both derivatives (to get the same plot)
Capture d’écran 2020-02-04 à 10 24 20

=> it looks better to me, and will provide less confusion. (with the first chart, it seems the original is almost always zero +- something)

  • First, it would be good to show you result, (that we can compare with original version)
  • Then, there is something I do not understand. You may see that the gradient of the sensor_value is high (always higher of the true_sensor if > 0) or 0.

=> 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 !

@afaucogney
Copy link
Contributor

I played a bit with jupyter, then this should more realistic to me.

Capture d’écran 2020-02-04 à 11 02 31

@balloob balloob added this to the 0.105.0 milestone Feb 4, 2020
balloob pushed a commit that referenced this pull request Feb 4, 2020
* 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
@basnijholt
Copy link
Contributor Author

Hi @afaucogney, you are right! I made a mistake in my plot. I both used 5*np.cos(xs) instead of 4*np.cos(xs), so the derivative shouldn't be larger than the function itself.

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 Δx → 0). That is the exact problem that I hoped to solve :)

I am doing some more small checks now.

Since now we essentially do a moving average, the derivative that we calculate is actually of t_now - time_window/2. I think we can still easily correct that too though.

@lock lock bot locked and limited conversation to collaborators Feb 5, 2020
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.

6 participants