Fix averageSince calculation

[J-N-K]

Average since is calculated over persisted values AND the actual value.
The description describes that it is calculated over persisted values OR
the actual value (if no persisted values are found). The description is
desribes the correct behaviour, while the implementation was wrong. This
is fixed with this PR.

Bug: #2404
Signed-off-by: Jan N. Klug jan.n.klug@rub.de

[watou]

I updated all the JavaDoc headers in #1854 but it has since been refactored. As mentioned in that PR, there are/were inconsistencies in what is returned across the various "since" methods that needed attention and documentation. Could this PR be expanded to complete that?

[J-N-K]

I agree.

• updatedSince, changedSince return true or false which is intuitive and correct
• maximumSince, minimumSince always include the current value. This will probably fail for some RRD
• averageSince returns the average of the persisted values including the current value
• sumSince uses only persisted values and returns ZERO if there are none, the current value is never included
• deltaSince, evolutionSince returns null if there are no persisted values

I think that all these functions should fail with null if no persisted values are found. Only deltaSince and evolutionSince should use the current value as this is necessary for the calculation. All aggregation functions will fail on Counter RRD because RRD4J always returns differences and never full values. If you include e.g. the packetcounter of an interface this will break al the calculations.

[watou]

Thanks @J-N-K! In addition to what makes sense if we were to rethink it now, we should also consider whether breaking OH1 compatibility is worthwhile (and if so, document the change(s) in OH2). Before the 1.0 release of ESH is a good time to go forward with the best plan.

[J-N-K]

There are three options:

• Leave it as it is. This is IMHO the worst one.
• Change averageSince in the proposed way of the PR. deltaSince and evolutionRate should be changed that they return either ZERO (possible for both) or the current value (deltaSince). This ensures that aggregation functions behave in the same way. OH1 compatibility is nearly preserved, except the wrong behaviour of averageSince is fixed.
• Change like proposed two posts above. This is more predictable and makes it possible to judge if persisted values exist in rules (null indicated there are no persisted values, if you get a DecimalType the value is valid). This breaks OH1 compatibility because the behaviour and the return types of the functions change. This is the best option to go in terms of what makes sense for the functions itself.

[maggu2810]

I would prefer to return null if a calculation could not be done because of e.g. missing values instead of return zero (and the caller does not know that there has been no input value).
IIRC the OH1 bundles are using the compat1x layer. Isn't it possible that that layer transform e.g. a null to zero if this is really necessary (but I assume that the target is to move OH1 bundles to OH2 ones some time).

[J-N-K]

This is an ESH bundle, the OH1 distribution will not be affected by any changes we make. It'll only break rules that are ported from an OH1 installation to ESH/OH2 without changes (in the case that no persisted values exist). Most probably this will lead to other problems anyway.

[J-N-K]

@watou, @kaikreuzer (as original author): what is your opinion?

[kaikreuzer]

I don't mind breaking OH1 compatibility if we can improve things here.
For functional changes (not for pure bug fixes), it would be great if you could also create a PR for https://github.com/openhab/openhab-docs/blob/gh-pages/tutorials/migration.md#rules.

But I have a few questions regarding the proposals:

Average since is calculated over persisted values AND the actual value.

Looking at the unit test that you changed in this PR, this behavior makes a lot of sense. If I KNOW the current value, why should it not be considered in the calculation? I do not expect 2007.5, when my current value is 3025.

I think that all these functions should fail with null if no persisted values are found.

You mean none at all (so we do not have any value for startDate) or no persisted values within the range? I do not see any issue with the current implementation with most the implementations.

Maybe it would make sense to come up with concrete unit tests that show a problem where everyone agrees that this is an unexpected result and we then go and fix the code so that those tests are green? As it is a complex matter and I feel that I already have "knots in my brain" (German proverb), I'd think this might be the best approach to move forward, because everyone can see his use cases covered without the risk that they are broken again by future changes.

[J-N-K]

Let's stick to averageSince for simplicity. Assume we have an item that get's persisted every five minutes.

10:00:00 value= 0.0
10:05:00 value=20.0
10:10:00 value=40.0

At 10:10:10 you ask for the averageSince(10:00:00). Your actual value is still 40.

current implementation: (0+20+40+40)/4 = 100/4 = 25
proposed implementation: (0+20+40)/3 = 60/3 = 20.0

Make a chart of the values and look at that. What do you think is the "better" average? IMO it's the second one.

Of course both methods will fail if the persistence interval is not fixed.From the mathematical point of view (and probably also from the expectation of the user) it would probably be best if you would calculate the integral (or an approximation to that) and divide by the timespan (formula). Then including the actual value would not hurt.

[kaikreuzer]

From the mathematical point of view (and probably also from the expectation of the user) it would probably be best if you would calculate the integral

I agree, I think this is the only correct solution to the problem - especially as it will also work if persistence is done on value change and not with a fixed frequency. It shouldn't be too hard to calculate as we only need to additionally consider the time between two values as well.

[J-N-K]

While I agree on the fact that it would not be so hard to implement and will perform well for non-aggregating persistence services as MySQL, it may (I have to think about that) impose problems for RRDs as they already average over the defined timespan (bin length, whatever you call it). Including the current value surely will fail for RRDs of COUNTER type, because there is no possible way to get the persisted (absolute/raw) values (the RRD4J API has no method for that).

This has been discussed here and here.

[J-N-K]

@kaikreuzer: Maybe adding the "includeCurrentValue" option is a good compromise. I have changed the averaging function as discussed above.

[kaikreuzer]

Maybe adding the "includeCurrentValue" option is a good compromise

I do not get the point of this. If we are now calculating the integral, why would you ever want to exclude the current state?

[J-N-K]

Because RRDs of counter type store differences, not actual values.

Value 1: 3458
Value 2: 3570
Value 3: 3572
Value 4: 3577
Value 5: 3580

Assume that you want averageSince from Value 2 up to now. RRD4J will return

12 2 5

Average is something like 6.2 (assuming equal time differences)

If you include 3580 you'll get something like 899 which is obviously wrong. There is no way to get the original values from the RRD4J API.

[kaikreuzer]

Average imho simply does not make sense for counter type and more specifically, the ESH persistence API does not have such a concept of "counter data".

[J-N-K]

Agreed. That is why RRD4J returns differences. Since data is stored over intervals you get something like consumption.

I store the values of my power meter. Of course the absolute value which is something like 75k kWh makes no sense. It nice to see it displayed in the UI, nothing else. Stored in a counter RRD I get power consumption within 5 minutes (or daily consumption in another RRD with a timespan of 1d per bin). Averaging these values makes a lot of sense, adding the current value does not.

The newly added switch is only 5-10 loc, that should not be a severe problem. And there is a use case for that.

[kaikreuzer]

The newly added switch is only 5-10 loc, that should not be a severe problem. And there is a use case for that.

The problem I have with it is that this "feature" is based on the assumption of a "misbehaving" persistence service, i.e. one that does not return what you have put in. As I said, this isn't a concept supported nor expected in ESH.

Looking at what you really want is an averageBetween(date1, date2) as you do not want to have it summed up to "now", i.e. the current state of the item. Would introducing such a method be a compromise?

[J-N-K]

Honestly I think that either the PR introducing counter RRDs should be reverted or RRD4J patched that it can return the current value or (better) replaced with a RRD implementation that is compatible with standard RRD tools.

[kaikreuzer]

the PR introducing counter RRDs should be reverted

Do you have a reference for that? I wasn't even aware that such a feature had been added.

[J-N-K]

openhab/openhab1-addons#2532

However, COUNTER RRDs are still the easiest way to graph usage of electrical power or data transfer. If this is removed a rule that calculates differences has to be created and these values need to be stored.

[kaikreuzer]

However, COUNTER RRDs are still the easiest way to graph usage of electrical power or data transfer.

Yeah, so I agree that it would be annoying to users to remove that feature again.
So why not follow my suggestion to introduce an averageBetweenmethod? This makes sense in ESH on its own and can be used for the (ESH-unsupported) counter databases.

[J-N-K]

I agree that this feature would be useful. The question is how this may solve the counter RRD problem. Imagine the following (date left out for simplicity):

persisted values;

10:00 15
10:05 20
10:10 17

current time: 10:13 19
will evaluate to:

averageSince(10:00) (5*(15+20)/2 + 5*(20+17)/2 + 3*(17+19)/2)/13
averageBetween(10:00, 10:12) (5*(15+20)/2 + 5*(20+17)/2 + 2*(17+???)/2)/12

How can I determine the value at 10:12 without using the current value? On the other hand, assuming a step from 15 to 20 at 10:05 and using the value from 10:00 for every time up to 10:04:59 as we already do for

averageSince(10:01) will evaluate to: (4*(15+20)/2 + 5*(20+17)/2 + 3*(17+19)/2)/12

is not EXACT either. So maybe using the last persisted value:

averageBetween(10:00, 10:12) (5*(15+20)/2 + 5*(20+17)/2 + 2*(17+17)/2)/12

is at least consistent.

[kaikreuzer]

ok, you convinced me that doing averages simply does not make sense as we do not have a continuum on those databases. The persistence service is simply not able to return a sensible value for a timestamp between two persisted values.

So my suggestion would be to forget about counters in this PR completely - changing the implementation to work with the integral for normal databases makes nonetheless a lot of sense!

[J-N-K]

Well, the original intent of this PR was to fix the counter issue as this is what I need.

[kaikreuzer]

Well, but the issue title is "Fix averageSince calculation" and fixing the implementation to correctly work with integrals perfectly fits here and solves a real bug. As you have implemented, it also makes sense to merge it, doesn't it?

[J-N-K]

There is a problem with the persistence tests if this if the current value is always included. The value of averageSince then depends on the time the test is run. Is it possible to set a time that shall be used for the tests?

[kaikreuzer]

Is it possible to set a time that shall be used for the tests?

Hm, I am afraid not. Maybe as a workaround you can check if the result is within some expected range instead of a concrete value?

[J-N-K]

I finally found a solution that should work. If this is ok for you, you can merge. Is it necessary that I squash or can you squash during merge?

[J-N-K]

I don't get why Travis fails.

[maggu2810]

Why don't you get it? Have you not seen the log or don't you think it is related to your changed?

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:0.23.1:compile (default-compile) on project org.eclipse.smarthome.model.persistence.tests: Compilation failure: Compilation failure:
[ERROR] /home/travis/build/eclipse/smarthome/bundles/model/org.eclipse.smarthome.model.persistence.tests/src/test/java/org/eclipse/smarthome/model/persistence/extensions/PersistenceExtensionsTest.java:[131]
[ERROR] long recentInterval = DateTime.now().getMillis() - endStored.getMillis();
[ERROR] ^^^^^^^^
[ERROR] DateTime cannot be resolved
[ERROR] 1 problem (1 error)
[ERROR] -> [Help 1]

[J-N-K]

I'll check tomorrow why it compiles for me.

[kaikreuzer]

this line should be removed again

[J-N-K]

done

[kaikreuzer]

please remove this empty line

[J-N-K]

done

[kaikreuzer]

Is this test stable enough? If the execution is slow, the average might be calculated with a much bigger value than recentInterval already and the results might differ, right?

[J-N-K]

I have adjusted it to 0.01 which should be good enough even for every slow execution. In my tests the difference was always around 0.0001, so a factor of 100 better than the value I have set now.

[kaikreuzer]

lgtm, thanks a lot!