Filter Sensor by dgomes · Pull Request #12650 · home-assistant/core

dgomes · 2018-02-25T01:24:49Z

Description:

Introduces the Filter Sensor platform (This PR comes in the aftermath of #12506)

This is a full rewrite using the Template Sensor pattern, and the configuration suggestion of @OttoWinter (thus supporting chaining).

Related issue (if applicable): fixes #12506

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

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

  - platform: filter
    name: "filtered realistic humidity"
    entity_id: sensor.realistic_humidity
    filters:
      - filter: outlier
        window_size: 4
        radius: 4.0
      - filter: lowpass
        time_constant: 10
        precision: 2

Checklist:

The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

robmarkcole · 2018-02-25T05:40:03Z

Looking great!

Couple of comments:

Are the outlier warnings useful? Can I use them to fine tune my filter (appears the radius variable could be tweaked)? If not, perhaps they shouldn't be logged as this will definitely result in forum threads titled 'filter spamming my logs'. Perhaps an attribute with the % of outliers (in addition to erasures) would be instructive.
self.states is a deque of previous sensor values, these aren't timestamped, but would it impact the functionality to capture the timestamp (e.g. using a dict)? Time info may be useful for other filters. This is a 'nice to have'.

Cheers

pvizeli · 2018-02-25T07:24:47Z

+})
+
+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Required(ATTR_ENTITY_ID): cv.entity_id,


I think this can be wrong interpreted from user. Maybe 'entity', 'state_from' is better?

I would be for entity: since the plural form of that (entities:) is already used for group: and the upcoming group platforms.

I used entity_id (singular) since that is what is used in template_sensor, min_max, history_stats (and I guess couple of other...)

So we either change all the others, or this is the common interpretation by users already...

dgomes · 2018-02-25T09:40:24Z

@robmarkcole can also move outliers into debug()

erasures -> numbers of outliers removed

robmarkcole · 2018-02-25T09:41:45Z

@dgomes numbers of outliers removed is just a number so not as helpful as a % IMO?

MartinHjelmare

Looks good overall.

I haven't looked at the tests.

MartinHjelmare · 2018-02-25T09:39:33Z

+PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
+    vol.Required(ATTR_ENTITY_ID): cv.entity_id,
+    vol.Optional(CONF_NAME): cv.string,
+    vol.Optional(ATTR_UNIT_OF_MEASUREMENT): cv.string,


ATTR_* constants should not be used in config schemas, but in service schemas. All constants in config schemas that represent keys should start with CONF_.

MartinHjelmare · 2018-02-25T09:42:33Z

+    vol.Required(ATTR_ENTITY_ID): cv.entity_id,
+    vol.Optional(CONF_NAME): cv.string,
+    vol.Optional(ATTR_UNIT_OF_MEASUREMENT): cv.string,
+    vol.Required(CONF_FILTERS): [vol.Any(FILTER_OUTLIER_SCHEMA,


Add cv.ensure_list.

MartinHjelmare · 2018-02-25T09:44:06Z

+
+
+@asyncio.coroutine
+# pylint: disable=unused-argument


This is already globally disabled.

MartinHjelmare · 2018-02-25T09:46:01Z

+    entity_id = config.get(CONF_ENTITY_ID)
+    filters = []
+
+    for _filter in config.get(CONF_FILTERS):


Don't use dict.get on required config keys. Just do dict[key].

MartinHjelmare · 2018-02-25T09:48:48Z

+        window_size = _filter.get(CONF_FILTER_WINDOW_SIZE)
+        precision = _filter.get(CONF_FILTER_PRECISION)
+
+        if _filter['filter'] == FILTER_NAME_OUTLIER:


Use CONF_FILTER_NAME.

MartinHjelmare · 2018-02-25T09:58:48Z

+                    self._state = filtered_state
+                    filt.states.append(filtered_state)
+                except ValueError:
+                    _LOGGER.warning("Could not convert state to number")


Please log what state failed the conversion.

MartinHjelmare · 2018-02-25T10:06:06Z

+    """
+
+    def __init__(self, name, window_size=1, precision=None):
+        """Initialize common properties."""


It's initializing attributes.

MartinHjelmare · 2018-02-25T10:06:50Z

+        """Initialize common properties."""
+        self.states = deque(maxlen=window_size)
+        self.precision = precision
+        self._stats = dict()


MartinHjelmare · 2018-02-25T10:08:00Z

+class OutlierFilter(Filter):
+    """BASIC outlier filter.
+
+    Determines if new state is in a band around the median


Add period at the end.

MartinHjelmare · 2018-02-25T10:15:47Z

+        }
+        for filt in self._filters:
+            state_attr.update({
+                slugify("{} stats".format(filt.name)): filt.stats


How will the nested dict that we create for state attributes look in the frontend?

this information is for fine tuning the filter options only. Should not be presented in the frontend.

State attributes are shown in a table under the history graph in the more info card of sensors.

Only alternative I see is to slugify the nested dict():

outlier_erasures: 123
lowpass_median: 50

comments ?

Probably easiest to flatten the dicts into one dict, by concatenating + slugify the key names for the inner dict with the holding key name. I think that's what you mean too?

dgomes · 2018-02-25T12:29:41Z

@robmarkcole you are right, changed! :)

MartinHjelmare · 2018-02-25T14:57:03Z

+        with assert_setup_component(0):
+            assert setup_component(self.hass, 'sensor', config)
+
+        self.hass.start()


I don't think you need to start home assistant in any of the tests.

MartinHjelmare · 2018-02-25T14:57:49Z

+            assert setup_component(self.hass, 'sensor', config)
+
+        self.hass.start()
+        self.hass.block_till_done()


Since nothing is tested after this, this is not needed.

OttoWinter · 2018-02-25T20:22:32Z

+DEFAULT_FILTER_TIME_CONSTANT = 10
+
+NAME_TEMPLATE = "{} filter"
+ICON = 'mdi: chart-line-variant'


I don't think the space belongs here (between mdi: and chart-[...]) :)

Also: Wouldn't it be better if we copied the icon from the base entity? I mean for me, the use case for this platform would be to improve some shitty raw sensor values that deviate way too much. For example, for the following sensor I'd like to have some smoothing. It would therefore be nice not to have to manually set the icon for every single filter platform.

good point, will get the icon from the "parent"

OttoWinter · 2018-02-25T20:23:20Z

+
+    sensors.append(SensorFilter(name, entity_id, filters))
+
+    async_add_devices(sensors)


Just a style suggestion: Why not just async_add_devices([SensorFilter(name, entity_id, filters)])?

OttoWinter · 2018-02-25T20:26:01Z

+        with assert_setup_component(1):
+            assert setup_component(self.hass, 'sensor', config)
+
+        self.hass.start()


Same as Martin's comment: you don't need to call hass.start()

OttoWinter · 2018-02-25T20:26:08Z

+        with assert_setup_component(1):
+            assert setup_component(self.hass, 'sensor', config)
+
+        self.hass.start()


OttoWinter · 2018-02-25T21:02:17Z

Sorry for bothering you this much :) Just another quick suggestion/use-case: I think another brilliant use of this sensor platform would be to have a filter that only emits a new state on every xth source state.

For example I have a simple temperature sensor outside that tracks the outside temperature, but it emits waaay too many values and makes the showing of the graph in the front-end really slow. If I understand correctly the current design for filters expects a new value to be spit out by the filter for every new state it receives (short off raising ValueError, but that spams the logs).

Maybe a bit out of scope for this PR, but it would be good to have a way for a filter to signal that it doesn't want to emit a new state. What do you think?

dgomes · 2018-02-25T21:23:50Z

Sounds simple enough, a SampleFilter, but let me get this PR in first 😁

BTW: several other filters are possible and easy to add, especially if we introduce scipy as a dependency :)

houndci-bot · 2018-02-25T23:46:40Z

+        else:
+            self._skip = True
+
+        return new_state 


trailing whitespace

houndci-bot · 2018-02-25T23:46:41Z

+        elif _filter[CONF_FILTER_NAME] == FILTER_NAME_THROTTLE:
+            filters.append(ThrottleFilter(window_size=window_size,
+                                        precision=precision,
+                                        entity=entity_id))


continuation line under-indented for visual indent

houndci-bot · 2018-02-25T23:46:41Z

+                                         time_constant=time_constant))
+        elif _filter[CONF_FILTER_NAME] == FILTER_NAME_THROTTLE:
+            filters.append(ThrottleFilter(window_size=window_size,
+                                        precision=precision,


continuation line under-indented for visual indent

dgomes · 2018-02-25T23:50:18Z

Added extra Throttle Filter (very simple, but required some foundation work)

@OttoWinter figured I could use the filter in my setup as well, so here goes an extra filter ;)

      - filter: throttle
        window_size: 10

It throttles to 1 sample per window, so you only need to define the window_size.

This reverts commit f4abdd3.

dgomes · 2018-02-27T22:42:09Z

I reverted... I just want the basic filter code through :)

balloob · 2018-02-28T01:27:43Z

+                filt_stat = "{}_{}".format(filt.name, filt_stat_key)
+                _LOGGER.debug("stats(%s): %s: %s", self._entity,
+                              filt_stat, filt_stat_value)
+                state_attr.update({


This is so expensive and inefficient. Create a dict with 1 value to then pass to the update method after which it is discarded? 🤔 You can do state_atr[filt_stat] = filt_stat_value but really, just go for the define it at once declaration I suggested earlier or explain to me why it is so important to log every attribute individually?

@robmarkcole and @OttoWinter in the previous PR made a case for having stats of the filters.

Personally I think logging is enough to tune the filters, logging can even be done in the filter itself saving the dict.

If I don't get feedback from you guys, I'm taking the stats off from this PR.

I say leave the stats of for now, since I coudn't even say what stats I'd request. Once I've been using the component a while then will be in a position to do some analysis and make a suggestion, cheers

I don't think we should do stat reporting through attributes (if at all). I personally hate it when entities put completely unnecessary stuff in their attributes that just clutter the front end (and state machine)

Then it's settled! :) uff ... one less feature to creep this PR

Databases all over the world are cheering 😉

balloob · 2018-03-01T00:25:45Z

+        @callback
+        def filter_sensor_state_listener(entity, old_state, new_state):
+            """Handle device state changes."""
+            self._unit_of_measurement = new_state.attributes.get(


Should this one fall back to the old unit if there is none in the current state? (just like icon)

filters have no units... they are pure math :)

while pure math might have an icon, makes no sense to have a default unit.

With old unit I meant the unit of measurement of the previous state that was processed. For history it's important that all states of an entity have the same unit.

What happens now is that a state will have unit set to None if the source state becomes unavailable/unknown. We should probably move setting this instance variable when we know we're going to adopt a filtered version of the state.

balloob · 2018-03-01T00:28:41Z

+                    if filt.skip_processing:
+                        return
+                    self._state = filtered_state
+            except ValueError:


Where is the conversion happening? Also, wouldn't it make more sense to try convert it once, before running the filters?

conversion happens inside filters that process numbers, moving the try catch there would replicate the same code several times...

balloob · 2018-03-01T00:29:52Z

+                _LOGGER.error("Could not convert state: %s to number",
+                              self._state)
+                self._state = STATE_UNAVAILABLE
+                return


Shouldn't we still update the state when this happens? (and thus not return)

#12650 (comment)

" just make sure that nothing is updated."

If you're not going to change anything, you don't have to assign self._state either.

balloob · 2018-03-01T00:30:34Z

+            if new_state.state in [STATE_UNKNOWN, STATE_UNAVAILABLE]:
+                return
+
+            self._state = new_state.state


I find this a very weird assignment as it is never used. I would prefer a local variable that is assigned to self._state at the end. That way if something unexpected blows up, you are not left with a temp variable as state.

its not temp, its the state of the previous filter

new_state.state is the state that triggered the state change. It has not been filtered yet. Since we have at least 1 filter, this assignment will never last when we update Home Assistant.

I would expect only to write values to self._state that you are intending to publish to Home Assistant. Class instance variables should not be used for temporary variables inside the scope.

balloob · 2018-03-01T00:35:31Z

+        window_size = _filter.get(CONF_FILTER_WINDOW_SIZE)
+        precision = _filter.get(CONF_FILTER_PRECISION)
+
+        if _filter[CONF_FILTER_NAME] == FILTER_NAME_OUTLIER:


You can do this easier. If you use the Registry decorator, you would have all classes by type in the registry. Then the config is already valid from the schema and maps to the keywords.

FILTERS = Registry() @FILTERS.register(FILTER_NAME_LOWPASS) class LowPassFilter(Filter): … for filter_args in filters: filter_args = filter_args.copy() filter_args['entity'] = entity_id filter_class = FILTERS[filter_args.pop(CONF_FILTER_NAME)] filters.append(filter_class(**filter_args))

balloob

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

Thank you for working through all my comments 🥇

dgomes · 2018-03-01T23:06:04Z

🎊 Was starting to loose hope!

B1tMaster · 2018-03-05T02:14:41Z

This is awesome. Really need filtering.. Will be great for Weather related sensors with Wind , temp etc..
currently charts from my Weather station looks really bad and wind gusts and direction shifts make data unusable.

Is it possible to add ability to exclude certain values (like ZERO ) from time series during calculations.
For example, if a sensor often dropping to zero and those are not really valid numbers that we want to be included in the calculations of the filter, would be great to have a flag to say: ignore all zeros.. or maybe values below certain threshold , or values that evaluate true with certain boolean condition.

dgomes · 2018-03-05T08:40:40Z

@B1tMaster this PR is already closed :)

I think your use cases are already covered by the basic filters, but feel free to add new ones ;)

dgomes added 3 commits February 25, 2018 00:46

filter sensor platform implementation

6fc5808

added tests

13c6c64

default arguments

f489bff

homeassistant added cla-signed new-platform platform: sensor.filter labels Feb 25, 2018

dgomes added 2 commits February 25, 2018 02:07

Fix for unavailable units during initial startup

865b866

unused variable

70124ce

homeassistant added the cla-signed label Feb 25, 2018

dgomes mentioned this pull request Feb 25, 2018

Filter Sensor home-assistant/home-assistant.io#4725

Merged

2 tasks

pvizeli reviewed Feb 25, 2018

View reviewed changes

MartinHjelmare requested changes Feb 25, 2018

View reviewed changes

Addresses code review by @MartinHjelmare

74ae520

homeassistant added the cla-signed label Feb 25, 2018

fix

c0a7f47

MartinHjelmare reviewed Feb 25, 2018

View reviewed changes

don't need hass in this test

81e7c3c

OttoWinter reviewed Feb 25, 2018

View reviewed changes

dgomes added 2 commits February 25, 2018 21:53

Various Improvements

4b56d7e

Added Throttle Filter

50da8ad

houndci-bot reviewed Feb 25, 2018

View reviewed changes

hound fixes

48a0ed0

test throttle filter

7b5ffe2

Address comments by @balloob

5a728c6

homeassistant added the cla-signed label Feb 26, 2018

dgomes added 4 commits February 26, 2018 20:50

added test, reformulated filter tests

e4461aa

Precision handling

f4abdd3

address comments from @balloob

ddfcf76

Revert "Precision handling"

7bf3eda

This reverts commit f4abdd3.

dgomes closed this Feb 28, 2018

dgomes reopened this Feb 28, 2018

balloob reviewed Feb 28, 2018

View reviewed changes

removed stats

fa232a0

homeassistant added the cla-signed label Feb 28, 2018

only round floats

11c3e52

balloob reviewed Mar 1, 2018

View reviewed changes

dgomes added 2 commits March 1, 2018 01:01

Registry decorator usage

5adf912

Tries to address remaining comments

423c683

balloob approved these changes Mar 1, 2018

View reviewed changes

balloob merged commit de3c769 into home-assistant:dev Mar 1, 2018

B1tMaster mentioned this pull request Mar 5, 2018

Investigate improving Wind Alerts reporting by using stats and history sensors B1tMaster/haas-config#79

Open

home-assistant locked as resolved and limited conversation to collaborators Mar 5, 2018

dgomes deleted the filter_sensor branch March 18, 2018 19:41

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


		sensors.append(SensorFilter(name, entity_id, filters))

		async_add_devices(sensors)

Uh oh!

Conversation

dgomes commented Feb 25, 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!

robmarkcole commented Feb 25, 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!

dgomes commented Feb 25, 2018

Uh oh!

robmarkcole commented Feb 25, 2018

Uh oh!

MartinHjelmare left a comment

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!

dgomes Feb 25, 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!

MartinHjelmare Feb 25, 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!

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

Uh oh!

Uh oh!

MartinHjelmare Feb 25, 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!

MartinHjelmare Feb 25, 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!

OttoWinter commented Feb 25, 2018

Uh oh!

dgomes commented Feb 25, 2018 •

edited

Loading

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

dgomes Feb 25, 2018 •

edited

Loading

MartinHjelmare Feb 25, 2018 •

edited

Loading

dgomes commented Feb 25, 2018 •

edited

Loading

MartinHjelmare Feb 25, 2018 •

edited

Loading

MartinHjelmare Feb 25, 2018 •

edited

Loading

dgomes commented Feb 25, 2018 •

edited

Loading

dgomes commented Feb 25, 2018 •

edited

Loading

dgomes Mar 1, 2018 •

edited

Loading