Improve Group Item QuantityType calculations

[andrewfg]

Fixes #4562

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]

Suggested change

	return baseUnit.isCompatible(memberUnit) || baseUnit.isCompatible(memberUnit.inverse());
	return baseUnit.isCompatible(memberUnit) || baseUnit.isCompatible(memberUnit.getBaseUnit().inverse());

We discussed this previously. I think we should always use the base unit for inversion, which works in all cases we have identified so far.

[andrewfg]

I guess you meant getSystemUnit() here?

[andrewfg]

PS Insofar as all the JUnit tests succeeded I am wondering if the change is necessary? Or should we add another test for that case?

[mherwege]

I guess you meant getSystemUnit() here?

Yes, that's what I meant.

PS Insofar as all the JUnit tests succeeded I am wondering if the change is necessary? Or should we add another test for that case?

Maybe try checking if mK (milliKelvin) toInvertibleUnit and then toUnit(Mired) works. I suspect that may give strange results as the inverse of milliKelvin is not in the same scale as Mired.

[andrewfg]

milliKelvin is wrong for two reasons .. a) the factor would be micro rather than milli, and b) the conversions are mirek = 1e6 / kelvin resp. kelvin = 1e6 / mired -- i.e. not mirek = microKelvin ..

EDIT i.e. it is mirek = micro(inverse(kelvin)) .. that is why it is called mirek micro reverse kelvin

[andrewfg]

Maybe also keep in mind that isCompatible() works both ways even without falling back to systemUnit. The issue was that toInvertibleUnit itself had a bug that requires an intermediate conversion via systemUnit.

So there is nothing to fix at this point in the code.

And #4561 provides the intermediate conversion via systemUnit.

[mherwege]

My intention was to not start from the system unit (K), but from a derived unit (like mK or whatever multiplier symbol). But you are right with the fix in toInvertibleUnit, it should be fine.

[mherwege]

My prior JUnit tests were testing toInvertibleUnit with target Kelvin and they all pass. However I added a test that tests toInvertibleUnit with the inverse target Mirek. This test currently fails. However it should work fine once #4561 is merged.

I don’t get why that fix would make a difference though. Did you try running the test with the patch applied as well?

[andrewfg]

Ok I will patch and test.

[andrewfg]

I just tried the tests with the #4561 code patch applied, and I can confirm that it works fine.

[mherwege]

Is color temperature really ever expressed in °C or °F? I know it now works, but I would just limit it to Kelvin and Mired.

[andrewfg]

I would rather keep the test. I know it should not be commonly used. But it addresses the common case where a user creates a plain vanilla Number:Temperature Item without specifying a unit. By default Number:Temperature items have unit=Fahrenheit in USA and unit=Celsius everywhere else. So we need to test that this won't fall over..

[mherwege]

Yes, I understand. I am still worried that fixing that issue actually makes things worse. Right now the conversion from °C to mirek fails in the conversion as it correctly says it is not the same scale. With the changes, it will succeed. But looking at all of the discussion in the community post, that same user also assumes the state description min and max are in Kelvin when he doesn’t set the unit. Without the error, he would not have known.

[andrewfg]

I think you are referring to #4432