Ignore negative derivative when the input is total_increasing#119141
Ignore negative derivative when the input is total_increasing#119141emontnemery merged 11 commits intohome-assistant:devfrom
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @Smankusors
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
home-assistant
bot
added
cla-needed
has-tests
integration: derivative
new-feature
small-pr
|
Hey there @afaucogney, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
Smankusors
force-pushed
the
non-negative-derivative
branch
from
5db9c54 to
0be2673
Compare
|
This should not need a configuration option. If we have a STATE_CLASS_TOTAL_INCREASING for the source sensor. We can assume to ignore negative values. |
|
What i mean is the derivative sensor should check the state class of the sensor, to know if it should ignore the negative value. |
hmm that could work but how about this Adguard Home sensor? It doesn't have a state class yet
this sensor resets every hour, but not to zero, but to... uh... last 24 hours maybe? will putting |
|
Well that sensor just looks weird. It resets to something strange. Just looks wrong. It should be corrected to some total sensor that reset to zero. |
frenck
added
the
smash
it seems to be just how the Adguard works I think https://community.home-assistant.io/t/adguard-home-dns-queries-decreases-hourly/602626 |
|
Well it should really be fixed in the adguard integration. Ignoring negative in integration integration is the wrong place to fix this. |
hmm I think I will go ahead with detecting in the meantime, for the Adguard Home sensor, is it possible to manually override it from the end user so that the sensor has a |
frenck
left a comment
frenck
left a comment
There was a problem hiding this comment.
Hi there @Smankusors 👋
There is now a merge conflict on this PR (caused by another PR that was merged). Could you take a look?
Thanks! 👍
../Frenck
frenck
removed
the
smash
to increase clarity of which negative I mean in case in the future we want a ignore_negative_value...
Smankusors
force-pushed
the
non-negative-derivative
branch
from
0be2673 to
1f05e37
Compare
Smankusors
changed the title
I figured that I could add this to the customize.yaml file, and it works 😀 I updated the PR now so that it now reads if the sensor is also I updated the docs over home-assistant/home-assistant.io#33152. But I wonder if I should tell the users to fix themselves if their sensor doesn't have |
| state = hass.states.get("sensor.power") | ||
| assert state is not None | ||
|
|
||
| assert round(float(state.state), config["sensor"]["round"]) == 2.00 |
There was a problem hiding this comment.
Instead of just the magic number 2.00, calculate it or add a comment explaining it
There was a problem hiding this comment.
at the second thought...
- I don't want to calculate it because it would duplicate the implementation in the unit tests. I want the tests to be calculation-free as much as possible so that they're independent of the algorithm.
- There's no need to comment because the test is already clear. My existing comment isn't unique to this unit test but applies to the entire component. The existing web documentation pointing to Wikipedia should suffice.
Therefore, I will remove this comment in the next commit. Wdyt?
| times = [20, 30, 40] | ||
| values = [10, 30, 0] |
There was a problem hiding this comment.
Let's extend with another sample to test the behavior of the state update after a discarded sample
Also remove the last comment because the test is already clear My existing comment there isn't unique to this unit test but applies to the entire component. The existing web documentation pointing to Wikipedia should suffice.
emontnemery
left a comment
emontnemery
left a comment
There was a problem hiding this comment.
LGTM, thanks @Smankusors 👍
…ssistant#119141) * if the derivative is negative, ignore it * add option to ignore the negatives or not * add tests for a new ignore negative derivative * add missing description when editing * rename to ignore_negative_derivative to increase clarity of which negative I mean in case in the future we want a ignore_negative_value... * use state_class=total_increasing to ignore the negative derivative * remove ignore negative from the config * add test for total_increasing_reset case * add comments * update test_total_increasing_reset with history tests Also remove the last comment because the test is already clear My existing comment there isn't unique to this unit test but applies to the entire component. The existing web documentation pointing to Wikipedia should suffice. --------- Co-authored-by: Erik Montnemery <erik@montnemery.com>
4 participants
Proposed change
The documentation here says that we can use Utility Meter integration if we have a sensor that periodically resets to zero. But I can't get it work to output changes per second just like the original issue.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: