Bayesian Binary Sensor by jlmcgehee21 · Pull Request #8810 · home-assistant/core

jlmcgehee21 · 2017-08-03T23:59:14Z

Description:

Why:

It would be beneficial to leverage various sensor outputs in a
Bayesian manner in order to sense more complex events.

This change addresses the need by:

BayesianBinarySensor class in
./homeassistant/components/binary_sensor/bayesian.py
Tests in ./tests/components/binary_sensor/test_bayesian.py

Caveats:
This is my first time in this code-base. I did try to follow conventions
that I was able to find, but I'm sure there will be some issues to
straighten out.

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

binary_sensor:
  name: 'currently_cooking'
  platform: 'bayesian'
  prior: 0.1
  probability_threshold: 0.7
  observations:
    - entity_id: 'switch.kitchen_lights'
      prob_given_true: 0.6
      prob_given_false: 0.2
      platform: 'state'
      to_state: 'on'
    - entity_id: 'sensor.stove_temperature'
      prob_given_true: 0.9
      platform: 'numeric_state'
      above: 100
    - entity_id: 'sensor.kitchen_motion'
      prob_given_true: 0.5
      prob_given_false: 0.2
      platform: 'state'
      to_state: 'on'

binary_sensor:
  name: 'in_bed'
  platform: 'bayesian'
  prior: 0.5
  probability_threshold: 0.95
  observations:
    - entity_id: 'sensor.living_room_motion'
      prob_given_true: 0.4
      prob_given_false: 0.2
      platform: 'state'
      to_state: 'off'
    - entity_id: 'sensor.basement_motion'
      prob_given_true: 0.5
      prob_given_false: 0.4
      platform: 'state'
      to_state: 'off'
    - entity_id: 'sensor.bedroom_motion'
      prob_given_true: 0.5
      platform: 'state'
      to_state: 'on'
    - entity_id: 'sensor.sun'
      prob_given_true: 0.7
      platform: 'state'
      to_state: 'below_horizon'

Checklist:

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

Documentation added/updated in home-assistant.github.io Add Bayesian Binary Sensor documentation home-assistant.io#3268

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.

homeassistant · 2017-08-03T23:59:16Z

Hi @jlmcgehee21,

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!

houndci-bot · 2017-08-03T23:59:23Z

closing bracket does not match visual indentation

houndci-bot · 2017-08-03T23:59:23Z

continuation line under-indented for visual indent

houndci-bot · 2017-08-03T23:59:23Z

continuation line under-indented for visual indent

houndci-bot · 2017-08-03T23:59:24Z

continuation line under-indented for visual indent

houndci-bot · 2017-08-03T23:59:24Z

'homeassistant.const.CONF_ENTITY_ID' imported but unused
'homeassistant.const.CONF_TYPE' imported but unused
'homeassistant.const.ATTR_ENTITY_ID' imported but unused

Why: * It would be beneficial to leverage various sensor outputs in a Bayesian manner in order to sense more complex events. This change addresses the need by: * `BayesianBinarySensor` class in `./homeassistant/components/binary_sensor/bayesian.py` * Tests in `./tests/components/binary_sensor/test_bayesian.py` Caveats: This is my first time in this code-base. I did try to follow conventions that I was able to find, but I'm sure there will be some issues to straighten out.

houndci-bot · 2017-08-04T00:24:21Z

'homeassistant.const.ATTR_UNIT_OF_MEASUREMENT' imported but unused
'homeassistant.const.TEMP_CELSIUS' imported but unused

houndci-bot · 2017-08-04T00:24:22Z

'homeassistant.const.ATTR_UNIT_OF_MEASUREMENT' imported but unused
'homeassistant.const.TEMP_CELSIUS' imported but unused

jlmcgehee21 · 2017-08-04T00:32:30Z

Was running tests locally with ptw tests/components/binary_sensor homeassistant/components/binary_sensor/ -- -x tests/components/binary_sensor/test_bayesian.py and had no trouble getting them to pass in isolation.

I probably will need some pointers regarding whatever side effects I may be causing when running the full test suite.

arsaboo · 2017-08-04T01:09:21Z

Love it....will be great for presence detection as well.

cmsimike · 2017-08-04T15:44:27Z

This will be fantastic to try to infer when my apartment should go into night/sleep mode!

Danielhiversen · 2017-08-05T08:20:38Z

+import homeassistant.helpers.config_validation as cv
+from homeassistant.components.binary_sensor import (BinarySensorDevice,
+                                                    PLATFORM_SCHEMA)
+from homeassistant.const import (CONF_NAME, STATE_UNKNOWN, CONF_SENSOR_CLASS,


CONF_SENSOR_CLASS does not exists in homeassistant.const anymore

Danielhiversen · 2017-08-05T08:24:24Z

+        else:
+            self.current_obs.pop(entity, None)
+
+    def __update_probability(self, prior, observation):


You are not using self inside the function, so the function could be extracted from the class.
We normally use only one _ in the beginning of the function name.

Oops, was updating self.probability before a refactor. Changed to a @staticmethod to keep the namespace of the class (my usual approach), if this is not preferred, I can move outside.

This change addresses the need by: * Removing `CONF_SENSOR_CLASS` and its usage in `get_deprecated`. * Make probability update function a static method, and use single `_` to match project conventions.

Danielhiversen · 2017-08-06T09:52:11Z

+    observations = config.get(CONF_OBSERVATIONS)
+    prior = config.get(CONF_PRIOR)
+    probability_threshold = config.get(CONF_PROBABILITY_THRESHOLD)
+    device_class = CONF_DEVICE_CLASS


Should this be config.get(CONF_DEVICE_CLASS) ?

jlmcgehee21 · 2017-08-15T13:46:44Z

@Danielhiversen, once I add docs in home-assistant.github.io, will this make the next release?

MartinHjelmare

Cool! Some comments below.

MartinHjelmare · 2017-08-15T19:30:58Z

+        BayesianBinarySensor(hass, name, prior, observations,
+                             probability_threshold, device_class)
+    ], True)
+    return True


setup_platform shouldn't return anything.

MartinHjelmare · 2017-08-15T19:54:15Z

+    vol.Optional(CONF_NAME, default=DEFAULT_NAME):
+    cv.string,
+    vol.Required(CONF_OBSERVATIONS):
+    vol.Schema([dict]),


You could add cv.ensure_list. Something like:

vol.Schema(vol.All(cv.ensure_list, [dict])),

It seems the only platforms that are supported are state and numeric_state. Shouldn't we try to check that the user configures the correct platforms? Right now a bad platform will cause an uncaught error.

MartinHjelmare · 2017-08-15T20:01:07Z

+
+        for obs in self._observations:
+            entity_id = obs['entity_id']
+            async_track_state_change(hass, entity_id,


Do this in the method async_added_to_hass which will be called when the entity is added to home assistant.

MartinHjelmare · 2017-08-15T20:13:19Z

+class BayesianBinarySensor(BinarySensorDevice):
+    """Representation of a Bayesian sensor."""
+
+    def __init__(self, hass, name, prior, observations, probability_threshold,


Don't pass in hass. It will be set on the entity when it has been added to home assistant.

MartinHjelmare · 2017-08-15T20:15:26Z

+                                         entity_observation.get('below'),
+                                         entity_observation.get('above'), None,
+                                         entity_observation):
+


Remove blank line.

MartinHjelmare · 2017-08-15T20:20:43Z

+
+    @asyncio.coroutine
+    def async_update(self):
+        """Get the latest data and updates the states."""


Write all the verbs in imperative mood: "Get... update..."

MartinHjelmare · 2017-08-15T20:22:45Z

+Use Bayesian Inference to trigger a binary sensor.
+
+For more details about this platform, please refer to the documentation at
+https://home-assistant.io/components/sensor.bayesian_binary/


https://home-assistant.io/components/binary_sensor.bayesian/

MartinHjelmare · 2017-08-15T20:39:18Z

+
+            prior = self.prior
+            for obs in self.current_obs.values():
+                prior = self._update_probability(obs, prior)


The signature of the _update_probability method has the parameters in the reverse order of this call. Is this deliberate? I'm not very familiar with the Bayesian theory in application.

Good catch! It actually didn't matter (and hence didn't fail my tests) because I was treating the scenario a bit naively (P(Event|False) = 1 - P(Event|True)). I thought this would be a decent first shot, but after some reflection, I preferred to allow the user to specify P(Event|False) and default to the above case if they do not.

MartinHjelmare · 2017-08-15T21:13:16Z

+    observations = config.get(CONF_OBSERVATIONS)
+    prior = config.get(CONF_PRIOR)
+    probability_threshold = config.get(CONF_PROBABILITY_THRESHOLD)
+    device_class = config.get(CONF_DEVICE_CLASS)


CONF_DEVICE_CLASS isn't part of your config schema.

houndci-bot · 2017-08-18T13:33:23Z

'homeassistant.const.CONF_STATE' imported but unused

pvizeli

Looks good

pvizeli · 2017-08-20T15:13:43Z

+            async_track_state_change(self.hass, entity_id,
+                                     async_threshold_sensor_state_listener)
+
+    def _update_current_obs(self, entity_observation, should_trigger):


add a function documentation. Add decorator "@callback" if they need run inside async loop or add "Async friendly" to function documentation if that dosn't matters.

I basically adapted the binary.threshold sensor and its test for this implementation, and it seems the test is made to run in what looks like a full hass environment. With this in mind, I would guess that the @callback decorator is not needed because my tests are passing? If this is true, then appropriate solution would be to add "Async friendly" to the docstring, correct?

As a side note, the pylintrc and the travis tests both designate docstrings are only needed for public methods. For ease of contribution, would it be appropriate to include modification of these files as part of my PR in order for linting to fail if these methods have no documentation?

I appreciate the feedback, and hope to address it the best way possible.

Good point. I make a PR to remove that from pylintrc. This small info about that function is in every case helpfull.

Yeah, the function can run async/sync. So but "Async friendly" to this function like https://github.com/home-assistant/home-assistant/blob/c56f99baafac33483dd13699993d7da4ee5d7efd/homeassistant/helpers/location.py#L11-L14.
That help to prevent on future change on that platform.

A async/sync problem is only visible on real world and not every time inside tests. It is very hard to find a problem like this.

👍 thanks for the clarification.

pvizeli · 2017-08-20T15:14:03Z

+        else:
+            self.current_obs.pop(entity, None)
+
+    def _process_numeric_state(self, entity_observation):


add a function documentation. Add decorator "@callback" if they need run inside async loop or add "Async friendly" to function documentation if that dosn't matters.

pvizeli · 2017-08-20T15:14:09Z

+
+        self._update_current_obs(entity_observation, should_trigger)
+
+    def _process_state(self, entity_observation):


add a function documentation. Add decorator "@callback" if they need run inside async loop or add "Async friendly" to function documentation if that dosn't matters.

jlmcgehee21 · 2017-08-28T00:12:50Z

@Danielhiversen @MartinHjelmare @pvizeli hopefully I have addressed all comments/concerns. I have also added documentation, and linked to the PR in the description for this PR. Unfortunately, it seems I missed the 0.52 release, but hopefully we can make the next one! Is there anything else I need to do?

MartinHjelmare

~~I'm not sure about the callback annotations, but the rest looks good.~~
Everything is good.

pvizeli · 2017-08-29T20:57:09Z

Retrigger travis API

pvizeli · 2017-08-29T21:53:35Z

Travis pass: https://travis-ci.org/home-assistant/home-assistant/builds/269760101

moskovskiy82 · 2017-08-30T06:33:11Z

Will this make into 0.53?

MartinHjelmare · 2017-08-30T06:33:43Z

Yes.

* upstream/dev: (113 commits) Fix fitbit error when trying to access token after upgrade. (home-assistant#9183) Upgrade sendgrid to 5.0.1 (home-assistant#9215) Upgrade pyasn1 to 0.3.3 and pyasn1-modules to 0.1.1 (home-assistant#9216) directv: extended discovery via REST api, bug fix (home-assistant#8800) Bayesian Binary Sensor (home-assistant#8810) Add cloud auth support (home-assistant#9208) Abode push events and lock, cover, and switch components (home-assistant#9095) Lint Sonarr tests Upgrade pymysensors to 0.11.1 (home-assistant#9212) Refactor rfxtrx (home-assistant#9117) Issue home-assistant#6893 in rfxtrx (home-assistant#9130) Support for season sensor (home-assistant#8958) Add counter component (home-assistant#9146) Fix and optimize digitalloggers platform (home-assistant#9203) Prevent error when no forecast data was available (home-assistant#9176) Add "status" to Sonarr sensor (home-assistant#9204) fix worldtidesinfo home-assistant#9184 (home-assistant#9201) Update pushbullet.py (home-assistant#9200) Fix dht22 when no data was read initially home-assistant#8976 (home-assistant#9198) Prevent iCloud exceptions in logfile (home-assistant#9179) ...

homeassistant added cla-needed new-platform platform: binary_sensor.bayesian labels Aug 3, 2017

balloobbot added the platform label Aug 3, 2017

houndci-bot reviewed Aug 3, 2017

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Aug 4, 2017

homeassistant added cla-signed labels Aug 4, 2017

houndci-bot reviewed Aug 4, 2017

View reviewed changes

minor cleanup

ec0deb4

homeassistant added the cla-signed label Aug 4, 2017

Danielhiversen reviewed Aug 5, 2017

View reviewed changes

jlmcgehee21 added 2 commits August 5, 2017 10:12

Address reviewer's comments

ad4a609

This change addresses the need by: * Removing `CONF_SENSOR_CLASS` and its usage in `get_deprecated`. * Make probability update function a static method, and use single `_` to match project conventions.

Address linter failures

7b3bf06

Danielhiversen reviewed Aug 6, 2017

View reviewed changes

fix device_class declaration

39465c1

Danielhiversen approved these changes Aug 11, 2017

View reviewed changes

MartinHjelmare requested changes Aug 15, 2017

View reviewed changes

MartinHjelmare reviewed Aug 15, 2017

View reviewed changes

homeassistant added the cla-signed label Aug 18, 2017

houndci-bot reviewed Aug 18, 2017

View reviewed changes

pvizeli requested changes Aug 20, 2017

View reviewed changes

homeassistant added cla-signed labels Aug 24, 2017

update doc strings and ensure functions are set up properly for async

a679a9b

homeassistant added the cla-signed label Aug 24, 2017

jlmcgehee21 mentioned this pull request Aug 28, 2017

Add Bayesian Binary Sensor documentation home-assistant/home-assistant.io#3268

Merged

MartinHjelmare approved these changes Aug 28, 2017

View reviewed changes

Make only 1 state change handle

ce28829

homeassistant added the cla-signed label Aug 29, 2017

fix style

29ca298

pvizeli approved these changes Aug 29, 2017

View reviewed changes

home-assistant deleted a comment from houndci-bot Aug 29, 2017

fix style part 2

066514d

homeassistant added the cla-signed label Aug 29, 2017

pvizeli approved these changes Aug 29, 2017

View reviewed changes

fix lint

a44bc31

pvizeli closed this Aug 29, 2017

pvizeli reopened this Aug 29, 2017

pvizeli merged commit 7de73e9 into home-assistant:dev Aug 29, 2017

balloob mentioned this pull request Sep 7, 2017

0.53 #9327

Merged

home-assistant locked and limited conversation to collaborators Dec 11, 2017

ghost added integration: bayesian and removed platform: binary_sensor.bayesian labels Mar 21, 2019


		self._update_current_obs(entity_observation, should_trigger)

		def _process_state(self, entity_observation):

Uh oh!

Conversation

jlmcgehee21 commented Aug 3, 2017 • 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!

homeassistant commented Aug 3, 2017

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!

jlmcgehee21 commented Aug 4, 2017

Uh oh!

arsaboo commented Aug 4, 2017

Uh oh!

cmsimike commented Aug 4, 2017

Uh oh!

Danielhiversen Aug 5, 2017 • 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!

Danielhiversen Aug 5, 2017 • 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!

jlmcgehee21 commented Aug 15, 2017

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!

MartinHjelmare Aug 15, 2017 • 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!

pvizeli 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!

pvizeli Aug 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jlmcgehee21 commented Aug 3, 2017 •

edited

Loading

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

Danielhiversen Aug 5, 2017 •

edited

Loading

Danielhiversen Aug 5, 2017 •

edited

Loading

MartinHjelmare Aug 15, 2017 •

edited

Loading

pvizeli Aug 20, 2017 •

edited

Loading

jlmcgehee21 commented Aug 28, 2017 •

edited

Loading

MartinHjelmare left a comment •

edited

Loading