Persistence extensions, calculate QuantityType sums in absolute values #4575
Conversation
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
| @@ -1799,7 +1807,8 @@ private static void internalPersist(Item item, TimeSeries timeSeries, @Nullable | |||
| Iterator<HistoricItem> it = result.iterator(); | |||
|
|
|||
| Item baseItem = item instanceof GroupItem groupItem ? groupItem.getBaseItem() : item; | |||
| Unit<?> unit = baseItem instanceof NumberItem numberItem ? numberItem.getUnit() : null; | |||
| Unit<?> baseUnit = baseItem instanceof NumberItem numberItem ? numberItem.getUnit() : null; | |||
| Unit<?> unit = baseUnit != null ? baseUnit.getSystemUnit() : null; | |||
|
|
|||
| BigDecimal sum = BigDecimal.ZERO; | |||
There was a problem hiding this comment.
You could do this in one line :)
Unit<?> unit = (baseItem instanceof NumberItem numberItem) && (numberItem.getUnit() instanceof Unit<?> numberItemUnit) ? numberItemUnit.getSystemUnit() : null;
| @@ -1402,7 +1409,8 @@ private static void internalPersist(Item item, TimeSeries timeSeries, @Nullable | |||
| BigDecimal deviation = dt.toBigDecimal().sqrt(MathContext.DECIMAL64); | |||
| Item baseItem = item instanceof GroupItem groupItem ? groupItem.getBaseItem() : item; | |||
| if (baseItem instanceof NumberItem numberItem) { | |||
| Unit<?> unit = numberItem.getUnit(); | |||
| Unit<?> baseUnit = numberItem.getUnit(); | |||
| Unit<?> unit = baseUnit != null ? baseUnit.getSystemUnit() : null; | |||
| @@ -75,6 +75,8 @@ public class PersistenceExtensionsTest { | |||
| public static final String TEST_GROUP_QUANTITY_NUMBER = "testGroupQuantityNumber"; | |||
| public static final String TEST_SWITCH = "testSwitch"; | |||
|
|
|||
| public static final double KELVIN_OFFSET = 273.15; | |||
There was a problem hiding this comment.
Minor point, but I would initialise this constant from QuantityType.valueOf(0, SIUnits.CELSIUS).toUnit(Units.KELVIN).doubleValue();
There was a problem hiding this comment.
Yes, correct. But that gives a series of potential null pointer warnings for toUnit. It can't be null, it polutes the warnings, and it is more code to avoid it. As this offset will never change, and this is slightly more performant anyway, I will keep it.
There was a problem hiding this comment.
Not going to fight it .. but just to be pedantic (it is a quiet day) .. a static final constant hardly has a performance hit .. and Objects.requireNonNull resolves such wrong null warnings .. but as I said not going to fight it. :)
| DecimalType dt = averageState.as(DecimalType.class); | ||
| Item baseItem = item instanceof GroupItem groupItem ? groupItem.getBaseItem() : item; | ||
| Unit<?> baseUnit = baseItem instanceof NumberItem numberItem ? numberItem.getUnit() : null; | ||
| Unit<?> unit = baseUnit != null ? baseUnit.getSystemUnit() : null; |
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Sum calculations in persistence extensions for QuantityType items where done, summing the values in the configured unit.
This creates a challenge for temperatures, where the result would be different depending on the configured unit. To keep the results consistent, the sums should be done in the system unit for the dimension, wich has an absolute zero.
This PR changes that.
The only impact would be for summing temperatures, whereby it is highly unlikely someone would be summing temperatures over a time series.
This relates to #4563 and would make the logic consistent between summing for groups and summing over persisted values.
This PR would also change the returned units for QuantityType variance and deviation calculations to the logically meaningful unit.