core icon indicating copy to clipboard operation
core copied to clipboard

Accept more than 1 state for numeric entities in Bayesian

Open HarvsG opened this issue 1 year ago • 8 comments

Continuation of stale #80268

Breaking change

Previously if more than one numeric_state entry was specified for a single entity, e.g for different ranges of increasing temperature, then the ranges which did not include the value would update the probability with the negative observation, this made configurations such as these almost unusable. However now, where more than one range is specified, only the range which contains the true value will update the probability.

When specifying above and below in numeric states, these are normally non-inclusive, however now if more than one range is specified for the same entity, below will become inclusive.

Proposed change

Support configs where probability varies over a number of different numeric ranges for example determining the probability that the bins are at the roadside over several ranges of decreasing signal strength. A number of issues arose

  • [x] When a value is on the boarder of two ranges eg 20 when (10,20),(20,30) which range should it fall in to, This was discussed here
  • [x] More validation is required to ensure ranges do not overlap, as it would be unclear how to handle overlapping ranges.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [x] Breaking change (fix/feature causing existing functionality to break)
  • [x] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue:
  • This PR is related to issue: fixes #79694 partially
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/33186

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [x] Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • [x] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

HarvsG avatar Jun 10 '24 10:06 HarvsG

Ready for review. @dgomes was the reviewer for the previous PR. (Sorry if the @ tag was the wrong thing to do)

HarvsG avatar Jun 15 '24 15:06 HarvsG

Can you quickly walk me through the need to check test_multiple_numeric_observations ?

As in the need for the test to exist? Essentially because that is the new feature added in this PR.

HarvsG avatar Aug 01 '24 13:08 HarvsG

Test is not failing on my system.

HarvsG avatar Aug 01 '24 13:08 HarvsG

Can you quickly walk me through the need to check test_multiple_numeric_observations ?

As in the need for the test to exist? Essentially because that is the new feature added in this PR.

As in explain what is being tested for documentation purposes :)

dgomes avatar Aug 01 '24 13:08 dgomes

As in explain what is being tested for documentation purposes :)

I have added a better description.

async def test_multiple_numeric_observations(
    hass: HomeAssistant, issue_registry: ir.IssueRegistry
) -> None:
    """Test sensor on numeric state platform observations with more than one range.

    This tests an example where the probability of it being a 'nice day' varies over
    a series of temperatures. Since this is a multi-state, all the non-observed ranges
    should be ignored and only the range including the observed value should update
    the prior. When a value lands on above or below (15 is tested) it is included if it
    equals `below`, and ignored if it equals `above`.
    """

HarvsG avatar Aug 01 '24 14:08 HarvsG

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Aug 05 '24 08:08 home-assistant[bot]

ruff is failing, please address the error message

dgomes avatar Aug 05 '24 13:08 dgomes

ruff is failing, please address the error message

@dgomes Should be fixed in 9a85bf8 which I snuck in seconds before your comment. (Github Workflow must be running a newer/different version of ruff than my pre-commit hooks). Many thanks for your speedy re-reviews :)

HarvsG avatar Aug 05 '24 13:08 HarvsG

Looks good, some minor comments on code style

Changes implemented

HarvsG avatar Sep 09 '24 21:09 HarvsG

@emontnemery, really sorry for the @, just wondering if you could spare a moment to re-review as I have another PR #122552 with merge conflicts that is about to come out of draft. It would be great if I could get this PR merged first so I can resolve conflicts before putting #122552 up for review.

Thank you and sorry again!

HarvsG avatar Sep 11 '24 21:09 HarvsG