GroupItem uses GroupItem's system unit

[andrewfg]

Fixes #4562
Requires #4561

Previously the calculations over a GroupItem's set of member Items were based on the Unit of the first member in the set. There are two problems: a) the order of the set is not fixed, and b) the Unit of the first member Item is also not definitive to the Unit of the expected result. Therefore these calculations could produce variable and unexpected results.

This PR normalises the calculations over the group's set of member Item so that all calculations are based on the Unit of the GroupItem's base Item.

Furthermore this PR allows conversion between non- inverted and inverted Units so invertible conversions (e.g. Kelvin <=> Mirek) are supported.

Signed-off-by: Andrew Fiddian-Green [email protected]

[andrewfg]

@mherwege for info..

[andrewfg]

@mherwege while testing this, I discovered that the JUnit assertion test values are physically WRONG. e.g. the sum of 23.54 °C plus 89 °C plus 122.41 °C is absolutely NOT 234.95 °C .. rather it is 781.24 °C .. this assumes that the three absolute Kelvin values must be summed. => So this really provokes the question about what we are really trying to achieve here? Do we want a) just the sum of three numbers, or b) the sum of three physical temperatures?

       items.add(createNumberItem("TestItem1", Temperature.class, new QuantityType<>("23.54 °C")));
        items.add(createNumberItem("TestItem2", Temperature.class, UnDefType.NULL));
        items.add(createNumberItem("TestItem3", Temperature.class, new QuantityType<>("89 °C")));
        items.add(createNumberItem("TestItem4", Temperature.class, UnDefType.UNDEF));
        items.add(createNumberItem("TestItem5", Temperature.class, new QuantityType<>("122.41 °C")));

        GroupFunction function = new QuantityTypeArithmeticGroupFunction.Sum(Temperature.class, null);
        State state = function.calculate(items);

        assertEquals(new QuantityType<>("234.95 °C"), state);

[mherwege]

@mherwege while testing this, I discovered that the JUnit assertion test values are physically WRONG. e.g. the sum of 23.54 °C plus 89 °C plus 122.41 °C is absolutely NOT 234.95 °C .. rather it is 781.24 °C .. this assumes that the three absolute Kelvin values must be summed. => So this really provokes the question about what we are really trying to achieve here? Do we want a) just the sum of three numbers, or b) the sum of three physical temperatures?

Look at the PR I already linked several times. This is on purpose. The second and third arguments are interpreted ad differential values, not absolute values. And that’s what makes sense, as most would expect that adding 20 °C to 20 °C gives 40°C. And with the way it is done, it does. It also works for Fahrenheit, which is not the same scale as Kelvin or Celsius. This problem is only there if the 0 point of the respective units is not the same.

[andrewfg]

The .. arguments are interpreted as differential values

Ok. Got it.

I just committed support for the following..

• use the units of the group base item when available (in particular applies to sum and probably median functions
• added support for inverse units (in particular applies to color temperatures)
• added unit tests for the above.

[mherwege]

What about the following approach:

Create a new method in QuantityType, addAbsolute, similar to add (

openhab-core/bundles/org.openhab.core/src/main/java/org/openhab/core/library/types/QuantityType.java

Line 585 in ce37425

public QuantityType<T> add(QuantityType<T> state) {

)

It should be something like:

    public QuantityType<T> addAbsolute(QuantityType<T> state) {
        Quantity<T> quantity = Quantities.getQuantity(this.quantity.getValue(), this.quantity.getUnit(),
                Scale.ABSOLUTE);
        Quantity<T> quantitytoAdd = Quantities.getQuantity(state.quantity.getValue(), state.quantity.getUnit(),
                Scale.ABSOLUTE);
        return new QuantityType<>(quantity.add(quantityToAdd));
    }

You then call this method instead of add in the group arithmetic functions.
This way, adding in the group will always use the absolute 0 reference and the result becomes independent of the units and sequence. This will also yield the right average.

This will then be different from when you simply do the calculation in a rule, because there only the first argument is treated as ABSOLUTE. But I don't think this kind of logic applies in groups anyhow.

[andrewfg]

You then call this method instead of add in the group arithmetic functions.

@mherwege please calm down. As you say add is a relative function. It is based on the zero point offset of the initial unit. So in the case of Celsius it is a relative add. And in the case of Kelvin (and most other zero based units) the add function is at the same time both relative and absolute.

So..

// absolute
sum = QuantityType.valueOf(0, Unit.KELVIN)
sum.add(celsius value)
sum.add(fahrenheit value)
sum.add(kelvin value)

// relative
sum = QuantityType.valueOf(0, Unit.Celsius)
sum.add(celsius value)
sum.add(fahrenheit value)
sum.add(kelvin value)

// etc.

[mherwege]

As you say add is a relative function. It is based on the zero point offset of the initial unit. So in the case of Celsius it is a relative add. And in the case of Kelvin (and most other zero based units) the add function is at the same time both relative and absolute.

Absolutely true, but my worry is the different results depending on the unit used. And that is not logical. QuantityType calculations are meant to give the same result, whatever the unit. And of course that cannot be true for invertible units, which is a special case. And that's what we try to solve that by converting to the group item base unit to make the result at least predictable. But if it breaks the rule of having the same result within the same dimension, whatever the unit, that is not acceptable. And the rule is broken because of the conversion to the base unit. Add now does not do a relative add anymore, whatever the unit.

[mherwege]

please calm down

Honestly, I am pretty calm on this. I am just trying to achieve the best possible result. That's why I keep on questioning things. If we don't do that now, I know we hit issues at some point in time and we will have to find other workarounds again.

[andrewfg]

my worry is the different results depending on the unit used

We just need to add the following text to the readme and JavaDoc; then everything clear :)

The sum resp. add function summates the series of values relative to the zero point offset of the target unit. Most units have an offset of zero in which case the result is an absolute sum of the values. However some units (e.g. Fahrenheit, Celsius) have a non zero offset; in these cases the result is a sum relative to the zero point e.g. 20 C + 30 C => 50 C

And that is not logical.

See. Perfectly logical. :)

if (non_zero_offset) {
   System.out.println("sum is relative to the offset");
} else {
   System.out.println("sum is ALSO relative to the offset");
   System.out.println("ergo: sum is absolute");
}

[mherwege]

The sum resp. add function summates the series of values relative to the zero point offset of the target unit. Most units have an offset of zero in which case the result is an absolute sum of the values. However some units (e.g. Fahrenheit, Celsius) have a non zero offset; in these cases the result is a sum relative to the zero point e.g. 20 C + 30 C => 50 C

This is not what the current add method in QuantityType does.
Here is an example:
20 K add 20 °C will result in 40 K
Equally 20 °C add 20 K will result in 40 °C

Within the group arithmetic functions, because you first convert all arguments to the base unit, you get (assuming base unit is K):
20 K + 20 °C = 20 K + 293.15 K = 313.15 K = 40 °C
Or if base unit is °C: -253.15 °C + 20 °C = -233.15 °C = 40 K
So the result becomes dependent on the base unit. I don’t think it should. So the only way around is to always use absolute values.

[andrewfg]

Equally 20 °C add 20 K will result in 40 K

Are you sure? I would expect a result of 40 °C ???

[mherwege]

Are you sure? I would expect a result of 40 °C ???

Indeed, 40 °C I will correct the post.

[andrewfg]

See the following unit tests. They all produce IMHO the correct results. Or do you expect them to produce others??.

• Sum Celsius Base
• Sum Kelvin Base
• Sum Kelvin Base with Mirek Input
• Sum Mirek Base with Mirek Input

[mherwege]

@andrewfg So, I ran the simulations and here is a little table. The original unit of the inputs are on top, but the values in the cells are already recalculated to the target unit. These correspond to the tests with some extensions.

				°C	°F	K	MK-1
1	testSumFunctionQuantityTypeDifferentUnitsBaseKelvin	1054.4	K	296.69	362.15	395.56
2	testAvgFunctionQuantityTypeDifferentUnitsBaseKelvin	328.4833	K	373.15	318.15	294.15
3a	testSumFunctionColorTemperatureDifferentUnitsBaseKelvin	8000	K	2000	2000	2000	2000
3b		6907.4	°C	1726.85	1726.85	1726.85	1726.85
4	testAvgFunctionQuantityTypeColorTempDifferentUnitsBaseKelvin	2000	K	2000	2000	2000	2000
5	testAvgFunctionQuantityTypeColorTempDifferentUnitsBaseMirek	500	MK-1	500	500	500	500
6a	testSumFunctionQuantityTypeDifferentUnits	234.95	°C	23.54	89	122.41
6b		1054.4	K	296.69	362.15	395.56
7a	testAvgFunctionQuantityTypeDifferentUnits	55.33333	°C	100	45	21
7b		328.4833	K	373.15	318.15	294.15

My first question is the difference in outcome between 6a and 6b (is the same as 1). In the current propsal, the base unit determines the way the sum is calculated. Before, the first element in the group did determine it. I would argue that even made the whole summing quantity types in groups unpredictable, but that is another story. At least now it is predictable.
My view was to always use absolute values, so always generate result (1 or 6b).

To that end, I also created a scenario 3b where the base unit is °C and there is a mirek input. So the sum would be off when you try to visualize this with e.g. a Kelvin state description pattern. This is another case you would have when the °C default is not changed.

I am just not so sure anymore it is the right thing to do to use group base item unit as the reference unit. It does make the result dependent on that unit setting. And so far the unit never really had an impact on any calculation we did. If someone changes the unit of an item, it has a broader impact than just this calculation. It will also impact persistence. That's why it got introduced in the first place. So if you change this to make the group sum calculation work, it impacts that as well. That's why I was somewhat advocating to always use the value against an absolute reference (hence the addAbsolute proposal). In that case base unit will not have an impact anymore.

The solution here is also loosly related to how to treat it for the state filter profiles. There is no natural base unit there. Summing in absolute units would avoid that problem there as well.

I don't want to be the final decision maker on this. I am not a maintainer, so won't be the final judge. I can live with your proposed solution as long as it is properly documented.

[andrewfg]

Thanks for the analysis. AFAICT this issue only affects the SUM calculation. The MIN MAX AVG and MEDIAN functions anyway produce the same results. @mherwege can you please confirm re MEDIAN? For SUM I think we have three possible ways forward:

1. SUM = relative sum based on GroupItem unit
2. SUM = absolute sum
3. Add two new GroupItem calculation functions SUMABS and SUMREL(*). Plus assign the existing SUM as alias for SUMREL to achieve largely the same results as before.

*) on the case of SUMREL the relative base would be the zero point of the GroupItem's Unit, rather than the Unit of the "first" item in the Group members set.

[mherwege]

The MIN MAX AVG and MEDIAN functions anyway produce the same results. @mherwege can you please confirm re MEDIAN?

Indeed, the challenge is only with SUM.

Following wiki is an interesting read: https://github.com/unitsofmeasurement/unit-api/wiki/Arithmetic-rules-for-Difference-versus-Absolute-quantities

This is actually the reference for all Quantity calculations and how they should be treated. I quote:

The golden rule is:

The numerical values of all arithmetic operations must be calculated as is all values were converted to system unit before calculation. The conversion (not the arithmetic) depends on the MeasurementType. For example the conversion of 0°C can be 273.15 K or 0 K depending if the measurement type is ABSOLUTE or INCREMENTAL respectively.

The QuantityType.add method treats the second argument in the add as a INCREMENTAL, and the first argument as ABSOLUTE. So it results in:
5 °C + delta 5 °C = 10 °C = 283.15 K, but also
5° C + delta 5 K = 278.15 K + delta 5 K = 283.15 K = 10 °C
So wether you convert before or after the addition does not matter.
This add method makes perfect sense in rules, where the second argument would typically be a differential. It makes no sense in group sums. All arguments need to be treated the same way.
With the current logic for group sums, we are getting:
target unit °C: 5° C + 5 °C = 10 °C = 283.15 K
target unit K: 278.15 K + 278.15 K = 556.30 K
The second result complies with the golden rule where all values are ABSOLUTE. The first one does not. And looking at it as if all are INCREMENTAL from a different reference depending on the unit, to me is a stretch where I fail to see the practical value. Also, QuantityType always wrap an ABSOLUTE Quantity, never an INCREMENTAL Quantity. So the fact that the value is a differential (INCREMENTAL) is lost.

If we step back for a moment, what would summing temperatures in a group really be used for? Honestly, I can't think of cases where it makes sense. Average or median, min or max I believe make a lot of sense. I am not sure summing temperatures in a group does to be honest. And unless someone finds an example where it does make sense, I would stick to the golden rule, which is convert everything to system unit before calculating. This avoids the extra complexity of adding (documenting, explaining, maintaining) another version of SUM, just for the temperature case, where I see little relevance.

So my vote goes to doing the calculations in system unit and not creating an extra flavor of SUM.

[andrewfg]

@mherwege I am good with your proposal above. But I thought it was you who was insisting that the calculations must be relative (20 C + 30 C = 50 C) so I am a little surprised that you seem to have changed your mind. ??

[mherwege]

@mherwege I am good with your proposal above. But I thought it was you who was insisting that the calculations must be relative (20 C + 30 C = 50 C) so I am a little surprised that you seem to have changed your mind. ??

Yes, and I was turning in circles on this. I have advocated the opposite in some of my messages as well. My appologies for that. My itch came from the fact that I went the extra mile to make rules work with first argument absolute and second argument differential (relative). And that's reflected in the add method. The moment the conversion is done beforehand, you actually make them relative to a new base, not the first argument anymore, as the conversion is always converting absolule values, not differential values. And then you treat the first argument as absolute and the second (converted) argument as relative again in the add method.
Anyway, after turning this around in my head so many times, my conclusion was that relative does not make sense in group calculations. So we should not attempt it, and use absolute in all cases by first converting to system unit.

I actually think an extra javadoc on the add method explaining this, stating that the first argument is treated as absolute and the second argument as differential would be appropriate. Do you mind putting that in? You are touching the same classes.

Also, this would anyhow be a breaking change. Currently the outcome depends on the unit of the first item in the group that is considered. I don't think that is fixed. With the current change, it becomes deterministic, whatever solution is chosen. It really doesn't matter if the current JUnit test still pass in that sense, as they are deterministic in that the first element in the group is fixed, and you fixed the tests by making the base unit the same as the unit of the first element in the list.

[andrewfg]

an extra javadoc on the add method explaining this, stating that the first argument is treated as absolute and the second argument as differential would be appropriate

I will do that.

[andrewfg]

@mherwege I made a commit just now that should hopefully reflect our final agreement.

[mherwege]

LGTM

[andrewfg]

@mherwege thanks for approving the PR. Unfortunately I had to revert your proposal from .collect(Collectors.toList()) back to .map(s -> (QuantityType) s).toList() .. the reason is that whilst the former would compile locally in Eclipse IDE it failed the Maven build. The reverted version passes both Eclipse and Maven.