Skip to content

Add Derivative component#26456

Merged
balloob merged 27 commits into
home-assistant:devfrom
afaucogney:feature/derivative
Jan 20, 2020
Merged

Add Derivative component#26456
balloob merged 27 commits into
home-assistant:devfrom
afaucogney:feature/derivative

Conversation

@afaucogney
Copy link
Copy Markdown
Contributor

@afaucogney afaucogney commented Sep 5, 2019

Description:

Here is the derivation component based on integration component.
This sensor will be useful:

  • to get a feeling about evolution of another sensor
  • to calculate the derivative of a sensor (ms -> m, kWh -> kW, m -> m/s)

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10323

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: random
    name: myRandom
    minimum: 0
    maximum: 10
    unit_of_measurement: v
  - platform: integration
    source: sensor.myRandom
    name: myIntegration
    unit_prefix: k
    round: 2
    unit_time: s
  - platform: derivative
    source: sensor.myRandom
    name: myDerivation
    unit: v/s
    unit_time: s
  - platform: derivative
    source: sensor.myDerivation
    name: mySecondDerivation
    unit: v/s2
    unit_time: s    

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

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

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

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

based on integration component
remove left and right
(was'n saved)
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @afaucogney,

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!

@afaucogney
Copy link
Copy Markdown
Contributor Author

Hi, this is my first PR for HomeAssistant, and I'm not a python dev.
So please help me going further with that PR

Comment thread homeassistant/components/derivation/sensor.py Outdated
@JeffLIrion
Copy link
Copy Markdown
Contributor

This platform should be called differentiation, not derivation.

@afaucogney
Copy link
Copy Markdown
Contributor Author

This platform should be called differentiation, not derivation.

@JeffLIrion are you sure of this. I m an not English, but for me differentiation does not take in account the time (so the dx). What do you think ?

@JeffLIrion
Copy link
Copy Markdown
Contributor

The process of finding a derivative is called differentiation. The reverse process is called antidifferentiation. The fundamental theorem of calculus relates antidifferentiation with integration. Differentiation and integration constitute the two fundamental operations in single-variable calculus.

https://en.m.wikipedia.org/wiki/Derivative

@tsvi
Copy link
Copy Markdown
Contributor

tsvi commented Sep 6, 2019

Or else call it derivative.

@afaucogney
Copy link
Copy Markdown
Contributor Author

@tsvi as I said I m not either fluent in Python or homeassistant framework, so my test case always get 0 as the value to assert, so my test failed. However it seems to work in a real configuration.. I almost copy + adapt the code from integration, but have no clue what is wrong. Could you help me ?

add test case
fix test values
still a prefix issue that should not
@afaucogney
Copy link
Copy Markdown
Contributor Author

@tsvi as I said I m not either fluent in Python or homeassistant framework, so my test case always get 0 as the value to assert, so my test failed. However it seems to work in a real configuration.. I almost copy + adapt the code from integration, but have no clue what is wrong. Could you help me ?

I fixed this, it was a question of rounding value.
However the prefix test doesn't work, and I have no idea why, any idea, help is welcome ? The implementation is the same as integration, but behave differently !

Fix test issue with unit of measurement

Co-Authored-By: Santobert <tobhaase@gmail.com>
@Spirituss
Copy link
Copy Markdown

Looking forward for the release. Very useful component.

unit_of_measurement,
):
"""Initialize the derivative sensor."""
_LOGGER.warning("unit_of_measurement: %s", unit_of_measurement)
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.

Remove this.


if unit_of_measurement is None:
self._unit_template = (
f"{'' if unit_prefix is None else unit_prefix}{{}}/{unit_time}"
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.

this is really unreadable. Please break it out into multiple statements.

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

This looks good. Ok to merge when final comments addressed and linting fixed.

@afaucogney
Copy link
Copy Markdown
Contributor Author

This looks good. Ok to merge when final comments addressed and linting fixed.

@balloob thanks for the approval. I'm so young in python that I did succeed to mock the line that trigger the error handling. And this is required to achieve the expected coverage.

For exemple line 104:

        if state:
            try:
                self._state = Decimal(state.state)
            except ValueError as err:
                _LOGGER.warning("Could not restore last state: %s", err)

I need a test that mock Decimal() to then throw a ValueError(). Could you give me an exemple, and then I will apply to other cases.
I succeed to achieve this on a simple exemple, but the async def test...
This PR is mainly a copy for the integration component, so I do not know all what I'm doing ... But I will fix the rest if I can find a solution for the test / coverage.

@thomasloven
Copy link
Copy Markdown
Contributor

In what case would decimal.Decimal() raise a ValueError?
I can not find any reference to it in the docs.

Comment thread homeassistant/components/derivative/sensor.py Outdated
@balloob balloob changed the title Derivation component Derivative component Nov 26, 2019
Move ValueError to SyntaxError
@afaucogney
Copy link
Copy Markdown
Contributor Author

In what case would decimal.Decimal() raise a ValueError?
I can not find any reference to it in the docs.

I moved to SyntaxError

@springstan
Copy link
Copy Markdown
Member

@afaucogney you have a couple of unused imports, please take a look at the Linting errors to spot and remove them from your code. Thanks!

@tkislan
Copy link
Copy Markdown
Contributor

tkislan commented Dec 16, 2019

@afaucogney Will you be finishing this PR? I would very much like to use it in my home automation

If not, I can fork your fork, and finish it.

@afaucogney
Copy link
Copy Markdown
Contributor Author

@afaucogney Will you be finishing this PR? I would very much like to use it in my home automation

If not, I can fork your fork, and finish it.

Hi @tkislan, thanks for your interest on my PR. I tried to fix some issue, if you think you may do better (I'm sure, cause I am a python novice), feel free to PR my branch, I will accept your contribution for sure !

Copy link
Copy Markdown
Contributor Author

@afaucogney afaucogney left a comment

Choose a reason for hiding this comment

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

Review done !

Comment thread homeassistant/components/derivative/manifest.json Outdated
Comment thread homeassistant/components/derivative/sensor.py Outdated
- update doc link
- migrate to general const import
@springstan
Copy link
Copy Markdown
Member

@afaucogney please fix your failing tests :)

@springstan springstan changed the title Derivative component Add Derivative component Jan 18, 2020
@balloob balloob merged commit b2212ad into home-assistant:dev Jan 20, 2020
@lock lock Bot locked and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.