Adding range filter by nielstron · Pull Request #13390 · home-assistant/core

nielstron · 2018-03-22T13:41:11Z

Description:

As there were issues with the outlier filter this adds a range filter on its own. It only allows values in a given range to pass.

Related issue (if applicable): additional way to solve #13363

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

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

sensor:
  - platform: filter
    name: "filtered realistic humidity"
    entity_id: sensor.realistic_humidity
    filters:
      - filter: range
        lower_bound: -30.0
        upper_bound: 50

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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:

Tests have been added to verify that the new code works.

houndci-bot · 2018-03-22T13:41:21Z

+        lower_bound (float): band lower bound
+    """
+
+    def __init__(self, window_size=1, precision=None, entity,


SyntaxError: non-default argument follows default argument

houndci-bot · 2018-03-22T14:15:36Z

+        upper = 20
+        filt = BandPassFilter(entity=None,
+                             lower_bound=lower,
+                             upper_bound=upper)


continuation line under-indented for visual indent

houndci-bot · 2018-03-22T14:15:37Z

+        lower = 10
+        upper = 20
+        filt = BandPassFilter(entity=None,
+                             lower_bound=lower,


continuation line under-indented for visual indent

houndci-bot · 2018-03-22T14:15:37Z


 from homeassistant.components.sensor.filter import (
-    LowPassFilter, OutlierFilter, ThrottleFilter, TimeSMAFilter)
+    LowPassFilter, OutlierFilter, ThrottleFilter, TimeSMAFilter, BandPassFilter)


line too long (80 > 79 characters)

houndci-bot · 2018-03-22T14:15:37Z

+        """Initialize common attributes."""
+        super().__init__(FILTER_NAME_OUTLIER, precision, entity)
+        self.states = deque(maxlen=window_size)
+


blank line contains whitespace

houndci-bot · 2018-03-22T14:15:37Z

-        self.states.append(filtered)
        return filtered

+class HistoricalFilter(Filter):


expected 2 blank lines, found 1

dgomes · 2018-03-22T14:22:24Z

I like the HistoricalFilter, but please keep the PR to one issue only.

dgomes · 2018-03-22T14:24:17Z

Also take into consideration that #13075 has rewritten the filters code to some extend

dgomes · 2018-03-22T16:15:01Z

+        self._stats_internal = Counter()
+
+    def _filter_state(self, new_state):
+        """Implement the outlier filter."""


Update the description

dgomes · 2018-03-22T16:15:58Z

+    """
+
+    def __init__(self, window_size=1, precision=None, entity,
+                 lower_bound=math.inf, upper_bound=-math.inf):


Don't need to set defaults. This is already handled by voluptuous

dgomes · 2018-03-22T16:17:02Z

Don't need to set defaults, voluptuous handles that

dgomes · 2018-03-22T16:18:39Z

I would avoid using math.inf (saving the math import) by just defaulting to None

I would like to keep this (or switch to float('inf') - saving a math import - or sth similar) because this is mathematically more elegant (the lower bound is negative infinite and thus not existant). With None more ugly comparisons are included in the source code.

Also since the newest update default values have to pass the voloptuous check too.

if lower_bound and value < lower_bound: return value

lets keep python 3.5 math.inf

I disagree. If there is no bound, we shouldn't check it.

if lower_bound is not None and value < lower_bound return value

Does None pass the validity check for float values in voluptuous?

Yes, Just mark the key as vol.Optional and not give any default.

fronzbot · 2018-03-26T19:59:39Z

A bit nit-picky, but "bandpass" filter is not a good name for this. It's really more like a thresholding filter. Bandpass implies it's getting rid of both high frequency content (noisy info or something) as well as low frequency stuff (ie. maybe some constant value).

Just figured I'd point that out seeing as we have a real lowpass filter already implemented, so sticking with the right nomenclature seems important (especially if a real bandpass filter is eventually implemented)

dgomes · 2018-03-26T21:40:26Z

I agree with @fronzbot, we should be rigorous with the naming to avoid misinterpretations by users.

nielstron · 2018-03-27T00:16:31Z

How about I split this filter into a minimum (/threshold) and a maximum value filter?Any name suggestions? Also what do you think of returning None for illegal values and letting the rest of the system do interpolation (instead of replacing the value with some specified value)

dgomes · 2018-03-27T00:19:22Z

See no need to split, just rename it to threshold filter or limit filter
returning None is not a good option, it will interfere with next in chain filters.

nielstron · 2018-03-28T17:39:26Z

I think splitting would lead to a more natural way of applying the filters, setting a maximum and a minimum separately. On my opinion the next in chain filters (or filters in general) should be able to handle None values (if only forwarding them).Regarding filters as some kind of mathematical procedure applied to values there has to be a default / fallback value where I think None is a good choice (as also handled well by home-assistant)

robmarkcole · 2018-03-29T05:08:00Z

Perhaps Range Filter or Bounded window

houndci-bot · 2018-03-30T12:03:57Z

continuation line over-indented for visual indent

houndci-bot · 2018-03-30T12:03:57Z

continuation line over-indented for visual indent

houndci-bot · 2018-03-30T12:03:57Z

continuation line over-indented for visual indent

dgomes · 2018-04-07T09:00:48Z

@nielstron history just got merged, you will need to fix the conflicts

nielstron · 2018-06-07T20:14:18Z

@dgomes What do you mean "lets keep python 3.5 math.inf"? Is there still a change requested?

dgomes · 2018-06-07T21:47:48Z

I was accepting the use of math.inf

but then balloob commented: #13390 (comment)

I think you should follow the comment

balloob · 2018-06-08T02:43:42Z

🎈

balloob · 2018-06-15T18:13:49Z

Any update on this?

dgomes · 2018-07-01T23:14:10Z

@nielstron can you make the change and we merge this :) ?

homeassistant · 2018-07-02T21:59:12Z

Hello @nielstron,

When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).

The commits that are missing a linked GitHub account are the following:

53976bdd5e7e9e482f69ce3af2d67321fe028def - This commit has something that looks like an email address (n.muendler@tum.de). Maybe try linking that to GitHub?.

Unfortunately, we are unable to accept this pull request until this situation is corrected.

Here are your options:

If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
```
git commit --amend --author="Author Name <email@address.com>"
```
  (substituting Author Name and email@address.com for your actual information) to set the authorship information.
- If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
  1. You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
  2. You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.

We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.

Thanks, I look forward to checking this PR again soon! ❤️

dgomes · 2018-07-02T22:08:24Z


    def __init__(self, entity,
-                 lower_bound, upper_bound):
+                 lower_bound=None, upper_bound=None):


There is no need to set *bound=None

homeassistant · 2018-07-03T09:06:24Z

Hello @nielstron,

When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es).

The commits that are missing a linked GitHub account are the following:

c5092f1c479993f59d41c69aecd909340b441d9b - This commit has something that looks like an email address (n.muendler@tum.de). Maybe try linking that to GitHub?.

Unfortunately, we are unable to accept this pull request until this situation is corrected.

Here are your options:

If you had an email address set for the commit that simply wasn't linked to your GitHub account you can link that email now and it will retroactively apply to your commits. The simplest way to do this is to click the link to one of the above commits and look for a blue question mark in a blue circle in the top left. Hovering over that bubble will show you what email address you used. Clicking on that button will take you to your email address settings on GitHub. Just add the email address on that page and you're all set. GitHub has more information about this option in their help center.
If you didn't use an email address at all, it was an invalid email, or it's one you can't link to your GitHub, you will need to change the authorship information of the commit and your global Git settings so this doesn't happen again going forward. GitHub provides some great instructions on how to change your authorship information in their help center.
- If you only made a single commit you should be able to run
```
git commit --amend --author="Author Name <email@address.com>"
```
  (substituting Author Name and email@address.com for your actual information) to set the authorship information.
- If you made more than one commit and the commit with the missing authorship information is not the most recent one you have two options:
  1. You can re-create all commits missing authorship information. This is going to be the easiest solution for developers that aren't extremely confident in their Git and command line skills.
  2. You can use this script that GitHub provides to rewrite history. Please note: this should be used only if you are very confident in your abilities and understand its impacts.
- Whichever method you choose, I will come by to re-check the pull request once you push the fixes to this branch.

We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community.

Thanks, I look forward to checking this PR again soon! ❤️

nielstron · 2018-07-03T09:07:54Z

There you go, sorry for the delay

dgomes · 2018-07-03T10:00:58Z

🥇

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

houndci-bot reviewed Mar 22, 2018

View reviewed changes

This was referenced Mar 22, 2018

Enable setting a median manually for the band-pass (outlier) filter #13386

Closed

Add description for band-pass filter home-assistant/home-assistant.io#4985

Merged

houndci-bot reviewed Mar 22, 2018

View reviewed changes

nielstron changed the title ~~Adding bandpass filter~~ Adding bandpass filter and overhauling filters Mar 22, 2018

homeassistant added the cla-signed label Mar 22, 2018

nielstron force-pushed the filter-band-pass branch from 8af6d02 to f5ea55d Compare March 22, 2018 14:27

homeassistant added the cla-signed label Mar 22, 2018

nielstron changed the title ~~Adding bandpass filter and overhauling filters~~ Adding bandpass filter Mar 22, 2018

homeassistant added the cla-signed label Mar 22, 2018

dgomes requested changes Mar 22, 2018

View reviewed changes

nielstron force-pushed the filter-band-pass branch from 62434dc to d0eafef Compare March 22, 2018 21:46

homeassistant added the cla-signed label Mar 22, 2018

homeassistant added the cla-signed label Mar 30, 2018

houndci-bot reviewed Mar 30, 2018

View reviewed changes

nielstron force-pushed the filter-band-pass branch from aed529f to ee022b9 Compare March 30, 2018 12:05

homeassistant added the cla-signed label Mar 30, 2018

nielstron changed the title ~~Adding bandpass filter~~ Adding range filter Mar 30, 2018

nielstron force-pushed the filter-band-pass branch from 8aff8ce to 5a8b8eb Compare April 9, 2018 19:13

nielstron added 3 commits June 6, 2018 20:40

Fixed wrong bound assignment on values below the lower bound

25f7c31

Removal of windows size and precision for range filter

8061f15

Fixed Rangefilter constructor call

33990ba

nielstron force-pushed the filter-band-pass branch from 564fcd4 to 33990ba Compare June 6, 2018 18:41

homeassistant added the cla-signed label Jun 6, 2018

balloob added the almost-done label Jun 8, 2018

homeassistant added the cla-error label Jul 2, 2018

Remove math.inf as bounds

fb65276

nielstron force-pushed the filter-band-pass branch from 53976bd to fb65276 Compare July 2, 2018 22:00

homeassistant added the cla-signed label Jul 2, 2018

dgomes reviewed Jul 2, 2018

View reviewed changes

homeassistant added the cla-error label Jul 3, 2018

Remove default none from filter sensor

6420ab5

nielstron force-pushed the filter-band-pass branch from c5092f1 to 6420ab5 Compare July 3, 2018 09:07

homeassistant added the cla-signed label Jul 3, 2018

dgomes approved these changes Jul 3, 2018

View reviewed changes

dgomes merged commit 66e33c7 into home-assistant:dev Jul 3, 2018

ghost removed the almost-done label Jul 3, 2018

home-assistant locked and limited conversation to collaborators Dec 10, 2018

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

Uh oh!

Conversation

nielstron commented Mar 22, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes commented Mar 22, 2018

Uh oh!

dgomes commented Mar 22, 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!

nielstron Mar 22, 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!

fronzbot commented Mar 26, 2018

Uh oh!

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

Uh oh!

Uh oh!

nielstron commented Mar 27, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgomes commented Mar 27, 2018

Uh oh!

nielstron commented Mar 28, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

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 Apr 7, 2018

Uh oh!

nielstron commented Jun 7, 2018

Uh oh!

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

Uh oh!

Uh oh!

balloob commented Jun 8, 2018

Uh oh!

balloob commented Jun 15, 2018

Uh oh!

dgomes commented Jul 1, 2018

nielstron commented Mar 22, 2018 •

edited

Loading

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

nielstron Mar 22, 2018 •

edited

Loading

dgomes commented Mar 26, 2018 •

edited

Loading

nielstron commented Mar 27, 2018 via email •

edited

Loading

nielstron commented Mar 28, 2018 via email •

edited

Loading

robmarkcole commented Mar 29, 2018 •

edited

Loading

dgomes commented Jun 7, 2018 •

edited

Loading