home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
69.97k stars 29.04k forks source link

Riemann Sum integration uses broken defaults #92404

Closed rkistner closed 6 days ago

rkistner commented 1 year ago

The problem

The trapezoidal method (the default) of the Reimann Sum integration is completely incompatible with the way Home Assistant stores values.

See:

I also see the issue come up on Reddit every now and then.

Every time the workaround is "use the left method". But it's not just those specific cases that need a different method - the default of trapezoidal is simply wrong for practically any use case. With the way Home Assistant does not store or report updates to states with the same value, it will give inaccurate results for any sensor that could have repeated values. And that would be pretty-much every sensor out there.

The current warning in the documentation is not enough - it is not just in some specific cases that left should be used - it should be used in practically all cases. And even the "Energy" example does not use left, and that is where we most often see the issue come up.

I'd recommend changing the default to left. Even though that could be considered a "breaking change", I'd estimate it would fix many more cases than it would break.

Alternatively, require always setting the method, and make it clear in the documentation that trapezoid should only be used in some specific cases. This requirement could be phased in over time, by first showing a deprecation message for config that doesn't specify the method.

The right method would probably be even worse, but at least most users should not attempt to use it.

Another related issue, but more difficult to solve:

What version of Home Assistant Core has the issue?

core-2023.4.6

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Riemann sum integral

Link to integration documentation on our website

https://www.home-assistant.io/integrations/integration/

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

home-assistant[bot] commented 1 year ago

Hey there @dgomes, mind taking a look at this issue as it has been labeled with an integration (integration) you are listed as a code owner for? Thanks!

Code owner commands Code owners of `integration` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign integration` Removes the current integration label and assignees on the issue, add the integration domain after the command.

(message by CodeOwnersMention)


integration documentation integration source (message by IssueLinks)

tefracky commented 1 year ago

Even the "left" method is not working:

- platform: integration
  source: sensor.heating_power_total
  name: "Heizung Energieverbrauch (Gesamt)"
  unique_id: energy_consumption_total
  unit_time: h
  unit_prefix: k
  method: left
  round: 3

The result is "unkown", but sensor.heating_power_total has the value 0.

rkistner commented 1 year ago

For left, it still only updates when the underlying value changes, but when it does update it will have the correct value.

For the issue of the update only happening when the value changes, see https://github.com/home-assistant/core/issues/88940. That is a more difficult one to solve - likely only by changing the implementation to be timer-based instead of / in addition to event-based.

With trapezoid, there is no way for the value to be correct without using a timer-based implementation. So having it as the default without fixing that is a big issue.

thecem commented 1 year ago

I think this is another issue, my integration was also working perfectly up to 2023.5.x. afterwards not more, I think this is relatet to the prolem that some values are not longer accepted due to ensure_ascii (Braking change) and numeric value problem like here:

https://github.com/robinostlund/homeassistant-volkswagencarnet/issues/474

LuckyTriple7 commented 1 year ago

Update the Riemann Entity with the Service Homeassistant Update Entity bring back the state:

service: homeassistant.update_entity data: {} target: entity_id: sensor.energieverbrauch_boiler

override80 commented 1 year ago

Is it possibile that it's related to this MR: https://github.com/home-assistant/core/pull/91975? Maybe sensors are not (yet) available at startup and this leads to these sensors being stuck in "unavailable" even when the source sensor becomes available for a long time. And if the source sensors reports no changes (like if the connected device is off, so consumption is stuck to 0), then it will be stuck in unavailable forever. The same changes have been applied before to utility_meter here: https://github.com/home-assistant/core/pull/91035/files.

dgomes commented 1 year ago

It would update on 1st sensor change

override80 commented 1 year ago

This is what is happening on my side:

Schermata 2023-05-10 alle 08 35 57

Schermata 2023-05-10 alle 08 36 07

Schermata 2023-05-10 alle 08 51 38

this is the implementation:

sensor:
  - platform: template
    sensors:
      smartplugs_consumption:
        friendly_name: Smartplugs Consumption
        icon_template: 'mdi:power-socket-eu'
        # TPLink, we delegate consumption calculation to total device nr (for different consumptions on/off
        value_template: >-
          {% set dev = namespace(value=0) %}
          {% set meross = namespace(total = 0) -%}
          # count plugs
          {% for pwr in states.switch -%}
            {% if pwr.entity_id | regex_search('^switch\.meross.*$') -%}
              {% set meross.total = meross.total + 1  -%}
            {% endif -%}
          {% endfor -%}
          # smart plug consumption is 0.8W each
          {% set dev.value = meross.total * 0.8 %}
          # let's add some 0.01 consumption to total consumption int every 5 mins to make riemann integration happy
          {% set current_minute = strptime(states('sensor.time'), "%H:%M").minute %}
            {% if current_minute % 5 == 0 %}
              {% set dev.value = dev.value | int + 0.01 %}
            {% endif %}
          {{ dev.value }}

        daily_smartplugs_rounded:
          value_template: >-
            {{ "%.2f"|format(states('sensor.daily_smartplugs_peak') | float) }}
          attribute_templates:
            cost: >-
              {{ "%.2f"|format(states('sensor.daily_smartplugs_peak') | float * 0.246) }}
            last_reset: "{{ now().replace(day=now().day -1, hour=22, minute=0, second=0, microsecond=0)  }}"
            state_class: 'total'
            meter_period: daily
          unit_of_measurement: kWh
          device_class: energy

  - platform: integration
    source: sensor.smartplugs_consumption
    name: smartplugs_instant
    unit_prefix: k
    round: 2
    method: left

utility_meter:
  daily_smartplugs:
    source: sensor.smartplugs_instant
    cycle: daily
    tariffs:
      - peak
      - offpeak

the entity in the picture is "daily_smartplugs_rounded", but the smartplugs_consumption (not rounded) has exactly the same saw-like pattern and value. though it get displayed correctly (i.e. positive) on the energy dashboard.

Don't know, maybe i'ts a templating error or something, Tried to set daily_smartplugs_rounded to state_class: 'total' or state_class: 'total_increment' (and remove last_reset, as per docs), but is still have this weird behaviour i did not have before 2023.5.

thecem commented 1 year ago

@frenck do you have some information what’s wrong with the integration, since 2023.5 it have strange behaviour and calculates to much. So the statistics renders to more an more useless.

any idea?

simowNgithub commented 1 year ago

@frenck do you have some information what’s wrong with the integration, since 2023.5 it have strange behaviour and calculates to much. So the statistics renders to more an more useless.

any idea?

same here :(

dgomes commented 1 year ago

There were several patches added since this thread started.

Answering with "strange behaviour" or "same" does not help to diagnose ... please open a new issue and fill in all the details, also include debug information.

FAB1150 commented 10 months ago

related question, why isn't it a timer-based implementation? only updating when the value changes introduces confusion and problems such as this one, numbers not changing until it receives a different value and the whole sensor becoming "unknown" when it receives a value of 0.

issue-triage-workflows[bot] commented 7 months ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

tefracky commented 7 months ago

The Problem still exists

EDelsman commented 6 months ago

The point the OP makes is valid, not just because HA does not store unchanged values. Battery sensors will typically only send a new value when the value changes significantly. So left Riemann is best for that too. Any sensor that updates on change - left is best.

Only on time based sampling trapezoid is better, but that is rarely the case. One could argue that time based sampling on something that requires a Riemann integral is by itself a bad choice unless the samples are very frequent.

Any sensor that has long constant values in HA need left. That is basically any sensor that can be 0 for a significant duration, such as wattage on devices, solar energy... again, most of what HA uses it for. And even if trapezoid would work, I would argue left would work pretty well too. The reverse is definitely not the case.

And did any one mention all graphs in HA also display the left Riemann equivalent, not trapezoid? That is not without reason.

issue-triage-workflows[bot] commented 3 months ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

FAB1150 commented 3 months ago

Bump

elupus commented 2 months ago

Is there any case where trapezoid would be significantly better than left? Ie where left would be very very wrong.

ps. I absolutely agree the default aught to change. I just need to get the back sides to a change in as well to try to convince others.

tefracky commented 2 months ago

@elupus In my case, the photovoltaics sensors are only updated every 15 minutes. Especially at cloudy days, there might be a significant change between two measurements/sensor updates. Hence, a trepezoid is much more accurate.

rkistner commented 2 months ago

@tefracky Do the sensors never have the same value twice in a row? What about at night?

tefracky commented 2 months ago

@rkistner Sure, the sensor have the same value at night. As a trick, you can use a very small random number (+ and -), so you can be sure the sensor changes and all integration methods work.

EDelsman commented 2 months ago

@elupus In my case, the photovoltaics sensors are only updated every 15 minutes. Especially at cloudy days, there might be a significant change between two measurements/sensor updates. Hence, a trepezoid is much more accurate.

I've heard this before, and as long as the original problem of this issue is not resolved, I still disagree. First of all, with 15 minute samples, accuracy is very low either way, especially with clouds. Second: seen over short periods of 15 minutes when there is changing levels of sun, it will be a bit more accurate. With left estimation the lower estimate when light levels rise is compensated by higher estimates when light levels drop. So on day level when there is variation in light levels the difference is minimal.

But third, the real deal breaker for trapezoid is always: when it is dark, the light levels are 0 and do not change for for instance 8 hours. The first measurement then coming in is interpreted as if it has been that way for 8/2 hours, while in fact it has been 15/2 minutes worth. That error will obliterate any perceived extra accuracy of trapezoid during the day.

The error you make is in the fact that if values do not change, you have no measurements, even if the sensor does "update". Home assistant does not store them and does not take them into account. Left method takes that fact into account, trapezoid does not.

dgomes commented 2 months ago

There are currently 2 open PR's that would solve some of the reported issues:

dgomes commented 6 days ago

Can we close this issue since 2024.7 release ?

elupus commented 6 days ago

Yes i think so. Trapezoidal as default is fine now that we trigger on reported values instead of changed only. If we could know if something is polled or pushed we could possibly do better, buy at the moment we dont know that.

rkistner commented 6 days ago

I'm happy to consider this solved, given the implementated changes