Initialise filter_sensor with historical values by dgomes · Pull Request #13075 · home-assistant/core

dgomes · 2018-03-10T23:57:41Z

Description:

Initialise filter sensor with historical values, therefore limiting the impact of restarts.

When HA is restarted the filter looses its current state (window), requiring a new running period until the filter becomes stable again.

This PR loads historical values that enable the filters to reach previous state upon initiation.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4883

Example entry for `configuration.yaml` (if applicable):

  - platform: filter
    name: "events"
    entity_id: sensor.power_smart_meter
    history_period: 00:15 
    filters:
      - filter: throttle
        window_size: 1
        precision: 2

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
Documentation added/updated in home-assistant.github.io
Tests have been added to verify that the new code works.

houndci-bot · 2018-03-10T23:57:46Z

missing whitespace after ','

houndci-bot · 2018-03-10T23:57:46Z

continuation line over-indented for visual indent

houndci-bot · 2018-03-10T23:57:46Z

missing whitespace after ','

houndci-bot · 2018-03-10T23:57:47Z

continuation line over-indented for visual indent

dgomes · 2018-03-13T12:18:45Z

@B1tMaster your comment is not related to the issue :)

I'm open to discuss further enhancements, lets please move this discussion to the forums ;)

balloob · 2018-03-15T04:15:54Z

Use dt_util.utcnow() - self._history_period instead

balloob · 2018-03-15T04:18:24Z

You're inside an async function and are calling a sync function: move this to the executor:

from functools import partial history_list = await self.hass.async_add_job(partial(history.state_changes_during_period, self.hass, start, entity_id=self._entity))

(you have to use partial because of the keyword parameter)

balloob · 2018-03-15T04:19:08Z

Don't use STATE_UNKNOWN. Use None instead, HA will do the right thing

balloob · 2018-03-15T04:22:28Z

This doesn't test anything. I would expect a new test that mocks history.state_changes_during_period to return a list of state objects that would then influence the filter data after setup.

I wasn't really testing the history feature 😅 it was just acknowledging the dependency

Will workout a proper test 👍

houndci-bot · 2018-03-15T23:28:34Z

trailing whitespace

houndci-bot · 2018-03-15T23:28:35Z

continuation line under-indented for visual indent

houndci-bot · 2018-03-15T23:28:35Z

line too long (101 > 79 characters)

houndci-bot · 2018-03-15T23:28:35Z

'datetime.datetime' imported but unused

balloob · 2018-03-16T02:06:02Z

We should not add this. Instead, check if history component is loaded inside the added_to_hass callback and only execute restore logic if it is. Home Assistant will make sure that if the user wants to enable the history component that it is set up prior to the sensor component.

Check like this:

if 'history' not in self.hass.config.components: return

balloob · 2018-03-16T02:09:50Z

I don't think that this should be configurable. We should just fetch based on the maximum window size of the filters.

Issue is: some filters have a time window, others have a states window (number of states in the window).

I can agree when the filter has a time window

If there is a states window I don't know the period of the sensor to derive the history_period (some sensors aren't even periodic) so my option was to let the user take control.

But then should we have at least some sane default ? For example, look for max window size or else set it to 30 minutes?

Can I work on the history component and add a:

def get_last_state_changes(hass, number_of_states, entity_id=None):
?

This method would basically query all previous states, order them descending, and limit the returned results to number_of_states. The results set would include the datetime of each state.

This method would enable me to solve this issue.

But wouldn't you still want to pass a window size when it's available ?

Should I open a new PR for the new function or use this PR (since its related) ?

Being able to determine the window size, I can't see any use case in which the user would define it's own window. Either he would define a larger window (wastes states) or a smaller window (would still be useful but would not properly restore the filter states)

I would not expect users to configure a window directly for this feature, however the user might define a window size for other filters, like time_sma.

This PR is fine.

Due to chaining there is only 1 window for all filters in the chain.

The code I will provide will query the window (time/number of state) of all used filters in the filter sensor. Merge all the states (probably through a set()) and initialize all the filters in the chain with the same states, thus reproducing the previous state.

OttoWinter · 2018-03-18T19:35:26Z

I'm not sure, but this might not work too well with #13104; because in there you're using now = int(dt_util.utcnow().timestamp()) even though the state changes are from the past. I think you could use the existing State#last_updated field.

Yes, the two PR's started at different times and will now require adequate merging

houndci-bot · 2018-03-19T00:09:27Z

blank line contains whitespace

Added get_last_state_changes()

houndci-bot · 2018-03-19T00:34:18Z

 from homeassistant.setup import setup_component
-from tests.common import get_test_home_assistant, assert_setup_component
+import homeassistant.core as ha
+import homeassistant.util.dt as dt_util


redefinition of unused 'dt_util' from line 8

houndci-bot · 2018-03-19T00:34:18Z

 from homeassistant.setup import setup_component
-from tests.common import get_test_home_assistant, assert_setup_component
+import homeassistant.core as ha
+import homeassistant.util.dt as dt_util


redefinition of unused 'dt_util' from line 8

# Conflicts: # homeassistant/components/sensor/filter.py

houndci-bot · 2018-03-21T00:14:44Z

-        state = self.hass.states.get('sensor.test')
-        self.assertEqual('20.25', state.state)
+                for value in self.values:
+                    self.hass.states.set(config['sensor']['entity_id'], value.state)


line too long (84 > 79 characters)

houndci-bot · 2018-03-21T00:14:44Z

+
+        ts = dt_util.utcnow()
+        for val in raw_values:
+            self.values.append(ha.State('sensor.test_monitored', val, last_updated=ts))


line too long (87 > 79 characters)

robmarkcole · 2018-03-23T07:13:45Z

@dgomes this is a very welcome PR. If I've understood correctly, it will return historical data as an array which is ideal for implementing the various scipy filters. I'm assuming the time stamps are available?

dgomes · 2018-03-23T14:40:02Z

@robmarkcole Yes you are correct! New filters can access as much backlog as they require with timestamps.

balloob · 2018-03-25T03:10:26Z

@robmarkcole note that we will not accept scipy as a requirement for the filter platform. It works without any requirement now and that's how it should stay.

B1tMaster · 2018-03-25T04:46:02Z

@balloob With open source scipy, one can develop so many new things related to ML, AI, data analytics. What is the reason of such hard stance against a most popular library for data-science ?
How about numpy? Would be nice to have a few of the data-science libraries to be available for use inside HA.

OttoWinter · 2018-03-25T06:07:28Z

@B1tMaster We already had that discussion here: #12506 (comment). In short, there's a difference between making it a core and a platform dependency + scipy is just to big and difficult to install to put in as a core dependency, because those have to be installed no matter if you use the features or not. Please keep further discussion limited to this PR. + Not as a filter dependency since the filter platform is probably used quite a.bit

dgomes · 2018-03-30T11:03:23Z

People are starting to work new filters, which will be impacted by this PR.

It's been 5 days since the last review, can we get some love :) ?

balloob · 2018-03-31T00:24:19Z

There are a 105 open PRs…

balloob

ok to merge once the isinstance usage is replaced with a variable to check the type.

homeassistant added cla-signed platform: sensor.filter labels Mar 10, 2018

houndci-bot reviewed Mar 10, 2018

View reviewed changes

homeassistant added the cla-signed label Mar 11, 2018

dgomes mentioned this pull request Mar 11, 2018

Improves documentation with better example and link to algorithm home-assistant/home-assistant.io#4883

Merged

2 tasks

This comment has been minimized.

Sign in to view

balloob reviewed Mar 15, 2018

View reviewed changes

Comment thread homeassistant/components/sensor/filter.py Outdated

Copy link
Copy Markdown

Member

balloob Mar 15, 2018

Choose a reason for hiding this comment

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

Use dt_util.utcnow() - self._history_period instead

balloob reviewed Mar 15, 2018

View reviewed changes

homeassistant added the cla-signed label Mar 15, 2018

houndci-bot reviewed Mar 15, 2018

View reviewed changes

balloob reviewed Mar 16, 2018

View reviewed changes

OttoWinter reviewed Mar 18, 2018

View reviewed changes

dgomes requested a review from a team as a code owner March 19, 2018 00:09

homeassistant added the cla-signed label Mar 19, 2018

houndci-bot reviewed Mar 19, 2018

View reviewed changes

Comment thread tests/components/test_history.py Outdated

Copy link
Copy Markdown

houndci-bot Mar 19, 2018

Choose a reason for hiding this comment

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

blank line contains whitespace

Initialise filter with historical values

e1280b7

Added get_last_state_changes()

dgomes force-pushed the filter_history branch from e15f01b to e1280b7 Compare March 19, 2018 00:28

homeassistant added the cla-signed label Mar 19, 2018

dgomes added 2 commits March 19, 2018 00:29

fix test

d374f90

merge

725974a

houndci-bot reviewed Mar 19, 2018

View reviewed changes

Major changes to accommodate history + time_SMA

71ae962

# Conflicts: # homeassistant/components/sensor/filter.py

houndci-bot reviewed Mar 21, 2018

View reviewed changes

hail the hound!

511342d

dgomes and others added 5 commits March 23, 2018 01:43

sensible default

333647c

No defaults in get_last_state_changes

c6cf099

list_reverseiterator instead of list

d9a83cb

prev_state to state

24bfa0e

update

bbd792f

dgomes requested review from rytilahti and syssi as code owners March 23, 2018 00:50

homeassistant added the cla-signed label Mar 23, 2018

dgomes force-pushed the filter_history branch from f004bd0 to bbd792f Compare March 23, 2018 00:57

homeassistant added the cla-signed label Mar 23, 2018

merge

49a7b4e

homeassistant added the cla-signed label Apr 3, 2018

balloob approved these changes Apr 6, 2018

View reviewed changes

dgomes added 2 commits April 6, 2018 22:02

added window_unit

7f05c48

replace isinstance with window_unit

e668f26

balloob merged commit 286476f into home-assistant:dev Apr 7, 2018

balloob mentioned this pull request Apr 27, 2018

0.68 #14124

Merged

dgomes deleted the filter_history branch May 7, 2018 22:05

home-assistant locked and limited conversation to collaborators Sep 5, 2018

ghost added integration: filter and removed platform: sensor.filter labels Mar 21, 2019

Uh oh!

Conversation

dgomes commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

dgomes commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robmarkcole commented Mar 23, 2018

Uh oh!

dgomes commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgomes commented Mar 10, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

balloob Mar 16, 2018 •

edited

Loading

dgomes Mar 16, 2018 •

edited

Loading

dgomes commented Mar 23, 2018 •

edited

Loading

B1tMaster commented Mar 25, 2018 •

edited

Loading

OttoWinter commented Mar 25, 2018 •

edited

Loading