Skip to content

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

Closed
nielstron wants to merge 5 commits into
home-assistant:devfrom
nielstron:filter-outlier-median
Closed

Enable setting a median manually for the band-pass (outlier) filter#13386
nielstron wants to merge 5 commits into
home-assistant:devfrom
nielstron:filter-outlier-median

Conversation

@nielstron
Copy link
Copy Markdown
Contributor

@nielstron nielstron commented Mar 22, 2018

Description:

Automatic computation of the median for the outlier filter lead to issues. This is an attempt at fixing it.
Also user can clearly define a band for passing values this way.

Related issue: fixes #13363 (but is not only meant as fix for this)

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: filter
    name: "filtered realistic humidity"
    entity_id: sensor.realistic_humidity
    filters:
      - filter: outlier
        window_size: 4
        radius: 4.0
        median: 50.0

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:

If the code does not interact with devices:

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

if (diff > self._radius):

self._stats_internal['erasures'] += 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line contains whitespace

else:
diff = abs(new_state - statistics.median(self.states))
if (diff > self._radius):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line contains whitespace

Copy link
Copy Markdown
Member

@OttoWinter OttoWinter left a comment

Choose a reason for hiding this comment

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

I honestly don't see why we should do this. Sure it would fix the issue, but the much much cleaner solution would be #13075.

Why should we add an additional configuration option if the other solution actually fixes the issue long-term?

if (self.states and
abs(new_state - statistics.median(self.states))
> self._radius):
if(self.states):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No parentheses for if in python:

if self.states:

"""Initialize Filter."""
super().__init__(FILTER_NAME_OUTLIER, window_size, precision, entity)
self._radius = radius
self._manual_median_enabled = (median == NO_MANUAL_MEDIAN)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why would you store this? I mean you can calculate it on the fly with self._manual_median.

@nielstron
Copy link
Copy Markdown
Contributor Author

nielstron commented Mar 22, 2018

This does not only fix #13363 but also enables freedom for the users. Maybe someone does want to limit the temperature sensor data to exactly -30 to 50 degrees celsius but the actual median of measured data is unknown. This allows for more control over which values to pass and which to keep. Running the statistics on historical data (as I understood #13075) is great too and should be added for median compuation.

Use the median if set manuall, not else
@dgomes
Copy link
Copy Markdown
Contributor

dgomes commented Mar 22, 2018

I think the use case #13386 (comment) is sound.

But I would simply set two variables: lower_bound, upper_bound (you could set both or just one)
In this situation, you would either set radius + window_size or lower_bound/upper_bound.

Fixing the median is very strange IMHO and looks little bit hackish.

@OttoWinter
Copy link
Copy Markdown
Member

I'm not sure whether my previous comment got posted (kind of on a flakey connection here...), so posting this again:

IMHO I don't really think the outlier filter shouldn't expose this functionality, as it's supposed (from what I understand) to adapt dynamically to the data, and not statically to some given values. I think the cleaner solution would be to introduce a new filter: range (kind of like what @dgomes suggested, but in a separate filter). With that you could set a min/max value that should be accepted in the filter chain. Any incoming value outside the range would abort the pipeline (and report an unknown state?)

Kind of like this:

sensor:
  - platform: filter
    name: "filtered realistic humidity"
    entity_id: sensor.realistic_humidity
    filters:
      - filter: range
        min: 10  # If not specified, just use max as upper bound. Could also be called lower_bound
        max: 80  # If not specified, just use min as lower bound.

I think this would make more sense because then the "dynamic" outlier filter would be separate from the "static" range filter (static in that it doesn't adapt to incoming values).

@nielstron
Copy link
Copy Markdown
Contributor Author

I have created a new bandpass filter that rather matches the idea of a range/basic band pass filter: #13390
Therefore will close this PR

@nielstron nielstron closed this Mar 22, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Outlier filter does not effectively handle situation where the first value is an outlier

5 participants