Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

RfC: How to treat addition/substraction for temperature values #5697

Closed
wants to merge 1 commit into from

Conversation

kaikreuzer
Copy link
Contributor

I came across this when thinking about the System Offset Profile: If a sensor value is not correct, you might want to adjust it by e.g. adding "0.5 °C" to it using that profile.

I have tried this within some rules for the moment. Adding 20 °C + 2 °C resulted in the expected 22 °C.

Considering that we cannot rely on a specific unit, it can definitely happen that temperature values with two different units are added. Doing a 20 °C + 2 K, the result is a surprising -251.15 °C. Clearly the calculation first converts the K to °C and then calculates the sum - but it does not really reflect the expectation of 22 °C here.

Reading https://physics.stackexchange.com/questions/132720/how-do-you-add-temperatures, the issue is that temperatures cannot be really added, one can only add/substract "temperature differences" - which unfortunately cannot be cleanly identified.

Nonetheless, when such a calculation is done, the only logical interpretation hence seems to be to treat the second operand as a temperature difference. And if we do so, we have to treat the above result as a bug - the added unit test (which fails) shows this situation.

As the code in question is in tec-uom and not in ESH - @keilw, could you please comment and either tell me where my reasoning is wrong or confirm that it is a bug that needs to be reported at tec-uom? Thanks!

Signed-off-by: Kai Kreuzer kai@openhab.org

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@keilw
Copy link

keilw commented Jun 7, 2018

At least one answer there is:

Just convert everything into kelvins (or Celsius, or Fahrenheit, it doesn't matter as long as you're consistent), then do the interpolation, then convert back if you need to. You will almost certainly find that this does work, and that it does give the same answer no matter which unit system you use.

This is pretty much what the RIs and derived implementations like uom-se do here.
Why would 20 °C + 2 K be 22 °C ?

However for a more detailed analysis I added a Unit Test for Temperatures.
Would like to hear input by @dautelle, @desruisseaux and maybe others who recently contributed like @filipvanlaenen or @andi-huber.

@desruisseaux
Copy link

It depends why one want to add temperatures. A value in °C or K may be an "absolute temperature" (T), in which case 20°C = 293.15 K. But the same value may also be a change of temperature (ΔT), in which case 20°C = 20°K. So how interpret 20°C + 2°C? It depends if the formula that we are trying to apply is:

  • T₁ + T₂ : there is some legitimate uses for such additions, but they are rare.
  • T + ΔT : a more common case.

There is no way to know if a JSR-363 Quantity represents T or ΔT; only the context can tell. Consequently there is no way to know how to perform the right addition with current JSR-363 API.

One possible fix would be to introduce a new object which represent explicitly the difference between two Quantity values, for example Increment:

  • Quantity - Quantity = Increment
  • Quantity + Increment = Quantity computed as in T + ΔT
    • 20°C + 10°C = 30°C
    • 20°C + 10 K = 30°C (same result because ΔT = 10°C = 10 K)
  • Quantity + Quantity = Quantity computed as in T₁ + T₂
    • 20°C + 10°C = 303.15°C (because T₂ = 10°C = 283.15 K)
    • 20°C + 10 K = -243.15°C (because T₂ = 10 K = -263.15°C)

Other operations need more though. If we want to be consistent with addition rules, it may be:

  • Quantity / Increment = Quantity<Dimensionless> computed as in T / ΔT
  • Quantity / Quantity = Quantity<Dimensionless> computed as it T₁ / T₂
    • 20°C / 2°C = 293.15 K / 275.15 K = 1.065…

@htreu
Copy link
Contributor

htreu commented Jun 11, 2018

I guess for your smarthome/iot application we are in the T + ΔT situation most of the time. As for the offset profile we just want to shift the value by an offset. The offset may be defined in any compatible unit though.

@htreu
Copy link
Contributor

htreu commented Jun 11, 2018

The question here is: Do we need to implement proper offset handling ourselves? Or does the uom library support us in any way?

@keilw
Copy link

keilw commented Jun 11, 2018

They seem very specific to Temperature, I'm not sure, if the JSR should ever support that. We have extension modules like https://github.com/unitsofmeasurement/uom-domain (e.g. support for MicroProfile Metrics and other use cases like Prometheus, etc.) so hard to say, if an API element like Unit or Quantity should get these options. If we do, it would likely come with JSR 385 only.

@desruisseaux
Copy link

This is not specific to temperature. This is an issue for all quantities having a non-zero offset in the conversion formula from "system unit". Temperature is the main example, but not the only one. We have at least:

  • Temperature, with °C defined as K - 273.15
  • Density sigma-t, with σT defined as ρ - 1000 kg/m³.

I'm familiar with density sigma-t because it is used in oceanography, but I guess that other fields in science have similar units too. To repeat myself: any unit having a "+" or "-" operation in their definition will have this problem with current JSR-363 API.

@keilw
Copy link

keilw commented Jun 12, 2018

I could not see JScience offering any approach to this either, at least not in the final v 4.x.
The method once called Unit.plus() (now it's shift()) also stated:

offset - the offset added (expressed in this unit, e.g. CELSIUS = KELVIN.plus(273.15)).

JScience does have a getExactAmount() and isExact() set of methods, but I could not spot methods or classes to deal with the above. UCAR which is even older and seems to have no activity in the last decade knows unit types like OffsetUnit with Celsius vs. Kelvin examples, but I could not find a concrete case like Celsius+Kelvin there either.
Whoever thinks it is worth adding to JSR 385, please create a Feature request in https://github.com/unitsofmeasurement/unit-api/issues (labelled enhancement) If something that does not add complexity or inconvenience to the API could be found (the current behavior would be the default but e.g. there could be additional arguments to shift() or add() for these cases) then we could add it to 385.

@desruisseaux
Copy link

Unit.shift or similar methods are for creating new unit definitions (i.e. modify the conversion formula); this is a separated issue than converting a quantity.

@desruisseaux
Copy link

Added an issue on unitsofmeasurement/unit-api#95

@keilw
Copy link

keilw commented Jun 14, 2018

Please use the sum(Unit) lambda function in uom-se QuantityFunctions that'll give you the desired result here, see QuantityFunctionsTest in the new RI for the same class.

@triller-telekom
Copy link
Contributor

The sum(Unit) lambda function converts both quantities into the given unit and afterwards adds them.

Thus, the following tests works, because it adds 1°C and 1K and results in 2°C.

https://github.com/unitsofmeasurement/indriya/blob/a630f7c0309f29351d60808be1e9c6df67ad733a/src/test/java/tech/units/indriya/function/QuantityFunctionsTemperatureTest.java#L78-L84

If you would change it to add 1°C and 1°F (this is not an absolute value but a relative offset), I would expect that it results in 1°C + 5/9°C = 1.55555555556°C But instead it calculates 256.9277777777778°C.

I have now implemented it in: https://github.com/eclipse/smarthome/pull/5679/files#diff-7971e9673feb59b9446980a49b516693R166 like this:

private static final @Nullable QuantityType<Temperature> ZERO_CELSIUS_IN_KELVIN = new QuantityType<>(0, SIUnits.CELSIUS).toUnit(SmartHomeUnits.KELVIN);
private static final @Nullable QuantityType<Temperature> ZERO_FAHRENHEIT_IN_KELVIN = new QuantityType<>(0, ImperialUnits.FAHRENHEIT).toUnit(SmartHomeUnits.KELVIN);

private @Nullable QuantityType<Temperature> handleTemperature(QuantityType<Temperature> qtState,
            QuantityType<Temperature> offset) {

        QuantityType<Temperature> finalOffset;

        // do the math in kelvin and afterwards convert it back to the unit of the state
        QuantityType<Temperature> kelvinState = qtState.toUnit(SmartHomeUnits.KELVIN);
        QuantityType<Temperature> kelvinOffset = offset.toUnit(SmartHomeUnits.KELVIN);

        if (offset.getUnit().equals(SIUnits.CELSIUS)) {
            finalOffset = kelvinOffset.add(ZERO_CELSIUS_IN_KELVIN.negate());
        } else if (offset.getUnit().equals(ImperialUnits.FAHRENHEIT)) {
            finalOffset = kelvinOffset.add(ZERO_FAHRENHEIT_IN_KELVIN.negate());
        } else {
            // offset is already in kelvin
            finalOffset = offset;
        }

        kelvinState = kelvinState.add(finalOffset);
        return kelvinState.toUnit(qtState.getUnit());
    }

@keilw
Copy link

keilw commented Jun 20, 2018

In the current version of uom-se this looks like a good workaround.
Please have a look at unitsofmeasurement/unit-api#95 if you haven't seen it already.
Your solution and experience may benefit a broader solution for the next (2.0) release of Unit-API and its RI Indriya.

@htreu
Copy link
Contributor

htreu commented Jun 21, 2018

Thank you all for the good discussion. I think with the solution @triller-telekom implemented we can now close here. We are looking forward to the next version of the unit API.

@htreu htreu closed this Jun 21, 2018
@keilw
Copy link

keilw commented Jun 21, 2018

Thanks for the update. Depending on whether the solution by @triller-telekom involves the Lambda sum() note, we plan to refactor that into the next version of Common Library (which uom-se also needs, thus you already use it under the hood) so other implementations like Eclipse UOMo, SIS, etc. may use it independently of the RI. See unitsofmeasurement/unit-api#100

@openhab-bot
Copy link
Contributor

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/rule-broke-with-latest-nest-binding/57106/2

@openhab-bot
Copy link
Contributor

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/rule-broke-with-latest-nest-binding/57106/4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants