unitsofmeasurement / unit-api

Units of Measurement API
http://unitsofmeasurement.github.io/unit-api/
Other
183 stars 42 forks source link

Clarify behavior of Quantity arithmetic on shifted units #95

Closed desruisseaux closed 5 years ago

desruisseaux commented 6 years ago

Clarify the behavior of Q₁ + Q₂ or Q * n (with n a unitless number) when the unit of measurement of Q, Q₁ or Q₂ is "shifted". Example of shifted units are:

Taking temperature in °C as an example, addition can happen in at least two different contexts:

In the T₁ + T₂ case, both quantities must be converted to K before addition. So 20°C + 10°C = 303.15°C because T₂ = 283.15 K. In the T + ΔT case, 10°C is an increment (ΔT). So in this later case 20°C + 10°C = 30°C.

In current API, there is no way to know if a number is an absolute quantity or an increment. So there is no way to know if we are in the T₁ + T₂ case or in the T + ΔT case. If we want to differentiate those quantities, we may need to:

Requires #98 Requires #99

desruisseaux commented 6 years ago

Note: see https://rechneronline.de/temperatur/temperature.php for example of operations on temperatures. Even if the above proposal is not apply, at least we should add tests making sure that 20°C + 10°C = 303.15°C, not 30°C, unless we clearly state in the javadoc a policy about when quantities are considered increments.

keilw commented 6 years ago

This sounds rather intrusive for adding it to the API and introduce something like javax.measure.Increment or add a new method to the UnitConverter interface in that same package. Especially if it does not apply to at least 90% of all quantities (in javax.measure.quantity plus e.g. si-quantity or systems-quantity) The mentioned other example Density is part of si-quantity but it does not seem used by any of the unit systems yet. We decided to keep Range or Measurement in implementation modules like Indriya. Or ComparableQuantity which in theory (Java SE8 and above now) could even be merged into Quantity, but I'm not sure about that either (will ask this in a separate ticket) So while using Comparable, Serializable or Prefix apply to pretty much every quantity (maybe the only ones where Prefix may not make much sense are Temperature and a few others) So a gut feeling tells me, the small amount of quantities (out of at least 60 in the 3 quantity modules/packages as of now, how many are confirmed to need Increment?) does not justify an extra API element, but it seems perfectly fine to define it similar to Range or ComparableQuantity.

Beside UnitConverter.convert() does not return a Quantity, so adding a method that does would break with its current purpose. If AddConverter (used between CELSIUS and KELVIN right now) does not work for this case, why not introduce another UnitConverter implementation that does? If a Quantity or similar element like Increment has to be applied and returned, the QuantityFunctions class looks like a good place for methods like delta() etc.

desruisseaux commented 6 years ago

I never proposed to have a method returning Quantity in UnitConverter. I proposed a deltaConvert(double) method, returning a double like the existing convert(double).

The other example is not Density, but sigma-t. Density has nothing special compared to other units; it sigma-t which is a shifted unit.

Yes, addition of an Increment interface is intrusive. But conceptually it applies to all units; not only temperature and sigma-t. It is not apparent for other units because in their case, Quantity and Increment have the same behavior.

The situation is not comparable to Range. Range can be defined in a totally independent way; supporting ranges in an external API do not require any change in JSR-363 API. But the situation is not the same for Increment. If arithmetic operations are defined in Quantity, then they need to define precisely their behavior with shifted units - current API is not satisfying. We can either talk about increment in the javadoc, or introduce an explicit Increment type.

keilw commented 6 years ago

The wording sounded a bit like the interface would be expected as a return value. Nevertheless if there were two methods there it had a rather significant impact on the Spec and other places to clarify how to use them and when. It is a bit like the new fireAsync() methods introduced with CDI 2 that are also applied and used in parallel to those already existing. The overhead in the API would not be that big, but Spec, RI and TCK would have to be changed significantly. Not entirely sure, if it had to be on the top level, but if you envision a Quantity.add(Increment) method then it must be, see Prefix and its relation to Unit.

Similar to CDI 2 I would probably call such new method UnitConverter.convertDelta() or UnitConverter.convertIncrement() rather than deltaConvert().

While Range could also be done using e.g. Google Guava, the Measurement interface is something e.g. OSGi Measurement has defined in its core API.

keilw commented 6 years ago

Wonder, if someone from the SmartHome team like @htreu or @kaikreuzer might help with that at least as Contributors?

desruisseaux commented 6 years ago

I'm find with convertDelta or convertIncrement method names.

Introducing an explicit Increment interface is one possibility. Another possibility is to not introduce new interface an introduce the following methods in Quantity instead:

keilw commented 6 years ago

While it adds extra methods that may actually be less intrusive and complex. https://github.com/unitsofmeasurement/unit-api/issues/96 asked about possibly adding a few methods now in ComparableQuantity into the API. If the result of these 4 candidates is always a Quantity, would method overloading with add(Quantity, OperationType) also work? OperationType would simply be an enum either standalone or a static inner type in Quantity.

Let's do a quick vote. Please state your preference for the result and argument of "increment" operations:

keilw commented 6 years ago

A) Add a new Increment type to the API.

keilw commented 6 years ago

B) Reuse Quantity and add new methods like addIncrement(Quantity) or add(Quantity, OperationType.INCREMENT) to it.

keilw commented 6 years ago

I hope others will also cast their vote. I voted B also because the whole formatting topic for quantities would get even more complicated if we had to differentiate between Quantity and Increment.

desruisseaux commented 6 years ago

Before to vote, is the problem clear to everyone? The mathematical problem (regardless any programmatic API), whether Increment would be sufficient of if there is a risk of need for more types, the pros and cons of different proposal?

For example is it clearly understood that whilemultiplyIncrement(Quantity) may seems less intrusive than multiply(Increment) + the addition of an Increment interface, it is also less convenient for the users because (s)he has to remember herself/himself if the last operation (s)he did resulted in an increment rather than a quantity?

C) Leave all types and method signatures as today, and add the following method in Quantity interface instead:

Where LevelOfMeasurement is an enumeration containing at least INTERVAL_SCALE and RATIO_SCALE (see https://en.wikipedia.org/wiki/Level_of_measurement#Interval_scale). Then the behavior of Quantity.add, subtract, multiply and divide depends on the LevelOfMeasurement of each quantity.

keilw commented 6 years ago

I know it's a little messy now, but I could update your comment and created an option C there. Which I find even less intrusive. Let's try to call it MeasurementLevel, regardless of being an inner enum or standalone type, to better blend in with existing types like MeasurementException.

filipvanlaenen commented 6 years ago

Note: an example of a legitimate use for T₁ + T₂ would be the calculation of the average over a set of temperatures (e.g. (T₁ + T₂)/2), but in that case, the offset is eliminated again by the division.

keilw commented 6 years ago

Thanks, guess that speaks for sticking to Quantity. Any preferences of A, B or C @filipvanlaenen ?

filipvanlaenen commented 6 years ago

Actually, I'd rather opt for D) Don't make a change (at least not in the API)

20°C + 10°C = 303.15°C sounds very counter-intuitive. I'm pretty sure this is going to produce a lot of bug reports.

Furthermore, I remember from thermodynamics back at university (a quarter of a century ago…) that temperature calculations were done in Kelvin, and nothing else. Not sure for physics, but I seem to remember that there too, you did the conversion into Kelvin when dealing with absolute temperatures.

I wonder whether we really have a problem for density sigma-t too, but the oceanographer would have to answer that.

Complicating factor: for integer quantity types, 20°C + 10°C will be 303°C, right?

keilw commented 6 years ago

I morphed your comment into option D. I take @filipvanlaenen D was your choice then?;-)

@unitsofmeasurement/experts and @unitsofmeasurement/contributors please consider voting on the three options, we have two thumbs up on the initial text, but that is not clear whether you prefer Option A, B or C to accomplish it.

keilw commented 6 years ago

@desruisseaux Would it be possible to vote on a choice (A-D) as well, since you presented 3 of them? @otaviojava, @htreu could you place your thumbs-up on one of the solution paths as well? I take it you don't prefer option D (don't do it at all) but we have to chose which of the options to go for if we want to do anything at all on this.

Thanks, Werner

desruisseaux commented 6 years ago

Yes, calculation in thermodynamic must be done in Kelvin. This is why 20°C + 10°C = 303.15°C if neither 20°C or 10°C are interval. Maybe 20°C + 10°C = 576.3 K would be less confusing.

Option D (no API change) would require that we choose a behavior for Quantity.add(Quantity) (e.g. is the argument considered an absolute measurement or an increment?) and we document it in the Javadoc. Users would need to think about whether that behavior fits his need.

I would go for a A amended by C. I.e. do everything described in A, except that the Increment type is replaced by a LevelOfMeasurement getLevel() method (maybe different name) in Quantity. Advantages:

desruisseaux commented 6 years ago

Isn't too early for vote? I'm not sure that we have well explored the proposed plan and alternatives. Each comment is raising new alternatives.

keilw commented 6 years ago

Well we can't have too many options. I was initially worried, it could better be dealt with in a custom or implementation-specific way. And @filipvanlaenen (or what he proposed as D) seems to share that concern. We should find out a trend (and of course like the "Compound" question those who are in favor of a certain approach also need to provide support or at least enough input on it)

kaikreuzer commented 6 years ago

In general, I would vote for D, i.e. do not handle increments at all. But what I then expect is a consistent behavior, but I think for this, something needs to be changed in the implementation. What I found in https://github.com/eclipse/smarthome/pull/5697 is that 0 °C + 0 °C = 0 °C while 32°F + 0 °C = 64 °F (=17.8 °C), which is simply unexpected from whatever perspective you look at it - thus I'd consider it a bug as you get different results depending on the unit that you used for the input.

20°C + 10°C = 303.15°C sounds very counter-intuitive. I'm pretty sure this is going to produce a lot of bug reports.

@filipvanlaenen If we do not introduce any increment handling, this would be the correct result, wouldn't it? So if you do not like that, why do you suggest to go for D?

dautelle commented 6 years ago

This is an interesting problem which is the same as for dates (often represented by a duration since January 1st, 1970). You don't add dates together (does not make senses) but you can add a duration to it in order to get a new date: Duration Since Jan 1st 1970 + Any Duration = Duration (implicit from Jan 1st 1970)

For our temperatures we have: Temperature above absolute Zero + Any Temperature = Temperature (implicit above absolute zero).

In other words: 10°C + 10° K = 20 °C but 10°C + 10° C = 10°C + 283 °K = 293° C

My choice is therefore D (nothing to do since I believe that it is the default behavior). You can understand this as quantities being always "delta/differences", in some cases a delta from an absolute predefined point.

kaikreuzer commented 6 years ago

since I believe that it is the default behavior).

False assumption. Check this unit test, which currently passes. It shows that 1 °C + 2 °C = 3 °C (and not 276 K as you claim). That's what I call the bug in the current implementation. It converts the second value always to the unit of the first one (instead of converting it to K) and then does the calculation. This results in the behavior that you get different results depending on the unit of your first value, which is simply wrong.

kaikreuzer commented 6 years ago

Just for the full picture: This unit test actually fails, while I think that we all agree that 1 °C + 1 K should result in 2 °C in ANY case (treating the values as absolute Kelvin values being added as well as treating the second value as an increment).

Failed tests: 
  NumberExtensionsTest.operatorPlus_Quantity_Quantity_withDifferentUnit:68 
Expected: is <2 ℃>
     but: was <-271.15 ℃>
dautelle commented 6 years ago

Ahah ! It is a bug !! In JScience we used to represent quantity internal values always in SI units (conversion was done at creation), that way no conversion is ever performed when adding/multiplying quantities (quantities values are added or multiplied together).

kaikreuzer commented 6 years ago

Ahah ! It is a bug !!

Just my words 😉

desruisseaux commented 6 years ago

I don't think that 1°C + 1 K = 2°C in any case. It depends if 1°C is an absolute temperature or an increment. We could decide that in A + B, the first operand (A) is an absolute temperature and the second operand (B) in increment, but this is an arbitrary choice.

Option D seems dangerous to me because it relies on convention (e.g. first operand is assumed absolute and second operand is assumed increment). If A or B is the result on another calculation, it can be anything. We expose the users to somewhat random behavior depending on the order in which he applies his operations. For example if A, B and C are absolute temperatures, then the behavior of the add method in A.add(B) should be different than the behavior of the same add method in A.add(B.subtract(C)). If we want to provide correct answer in those two cases, then we need to know if a quantity is an increment or not (either with subtypes or with enumeration values). For example B.subtract(C) would return a Quantity with LevelOfMeasurement set to interval, so that A.add(…) can use this information for switching to the proper behavior.

dautelle commented 6 years ago

Actually, the meaning of the quantities is up to the user. You can do B + A (with B being an increment and A the increment above the absolute zero), it will lead to the same result. If a user wants to hold a variable which is a temperature above the melting point of lead, why not! If he adds two such variables together it will not make senses.

keilw commented 6 years ago

@dautelle Can you point to where JScience does or did this? While there are many often patched versions of JScience 4 all over GitHub, this https://github.com/javolution/jscience must be the SNAPSHOT version of the recent codebase. Except an abstract base class Amount I did not find anything similar to the JSR 363 Quantity. JSR 363 does not store everything in SI units when a Quantity is created.

If you do A+B then B is converted into A as long as the quantity type is compatible.

dautelle commented 6 years ago

Yes it is the Amount classs (http://jscience.org/api/org/jscience/physics/amount/Amount.html)

desruisseaux commented 6 years ago

@dautelle yes, the meaning of the quantities is up to the user. Which is why we need either a way to tell to Quantity.add(…) how to interpret the quantity (options A, B and C), or adopt a convention based on argument order (my interpretation of option D).

kaikreuzer commented 6 years ago

Option D seems dangerous to me because it relies on convention (e.g. first operand is assumed absolute and second operand is assumed increment).

I understood option D as not supporting increments at all, but treating all values as absolutes. I thought that A+B were about introducing increment handling.

Whatever: I still vote for option X, which is to treat all as an absolute, which would require to do all temperature calculations only after converting all values to K.

dautelle commented 6 years ago

Agree with option X, but considering all quantities are increment (which can be an increment above the absolute zero == absolute value).

desruisseaux commented 6 years ago

Treating all values as absolute: 1°C + 2°C = 276.15°C. Treating all values as increment: 1°C + 2°C = -270.15°C (edit - see below)

desruisseaux commented 6 years ago

Ah sorry, my last comment is wrong. 1°C + 2°C = 3°C since the result is also an increment.

keilw commented 6 years ago

In this GitHub mirror: https://github.com/tarelli/jscience/blob/master/src/org/jscience/physics/amount/Amount.java I cannot see any explicit conversion from the given Unit into a SI unit. It is possible, because JScience does not support more than what JSR 275 did back then, there are mostly SI units available to use, but both CELSIUS and KELVIN were defined in https://github.com/tarelli/jscience/blob/master/src/javax/measure/unit/SI.java (beside several non-SI units like BIT or BYTE ;-)

dautelle commented 6 years ago

https://github.com/javolution/jscience/blob/master/physics/src/main/java/org/jscience/physics/amount/Amount.java

dautelle commented 6 years ago

Note: Implementation is not on GitHub yet (migration pending)

keilw commented 6 years ago

I would say X is up to a consuming system like SmartHome or others, I would not force even the RI to do this under every circumstance. What I could imagine, that's why I mentioned Indriya QuantityFunctions, is something like an addConverted(Unit targetUnit, Quantity... quantities) method that would add up two or more quantities after explicit conversion. In theory that could also be something like Quantity.add(Quantity, Unit) in the API.

A new function in QuantityFunctions could even be added to uom-se (since Indriya is largely based on it) so if that kind of enforced conversion helped certain aspects of SmartHome and other apps it won't have to wait for version 2.0 of the JSR.

Actually I think we already have a similar function ;-) sum(Unit<Q> unit) in QuantityFunctions does q1.to(unit).add(q2.to(unit) in a Lambda fashion. @kaikreuzer would that work?

keilw commented 6 years ago

Implementation of JSR 363? I still haven't heard from Harold about our demand for a list of compatible implementations at JCP.org (similar to what e.g. 107 got) but if the JCP does not have the resources for that any more, we could still use the UoM GitHub site.

filipvanlaenen commented 6 years ago

A few more comments:

I'm still there that people who handle temperatures should know there stuff. Adding temperatures doesn't make sense, just like adding dates doesn't make sense, unless you're trying to calculate an average. Therefore, I think 1 °C + 2 °C should be 3 °C.

Should we also consider the behaviour of logarithmic units if we're going to change the API?

That being said, if the choice is between A, B and C, I'm leaning towards A.

kaikreuzer commented 6 years ago

Therefore, I think 1 °C + 2 °C should be 3 °C.

The whole point of this library is to be able to do unit-safe calculations. So the question you will have to answer is what you think 1 °C + 1 °F + 1 K is and whether it is the same as 1 K + 1 °F + 1 °C.

keilw commented 6 years ago

If you use the sum(Unit) Lambda in uom-se as mentioned above (it was there already, Indriya has not added it) then you can take a List or array of 1 °C, 1 °F, 1 K and sum them all in K (or another matching unit) and do the same for any given order of quantities. The result must be the same.

Can't use F in that case (because Indriya doesn't know it), but the test for the Lambda sum() function: https://github.com/unitsofmeasurement/indriya/blob/master/src/test/java/tech/units/indriya/function/QuantityFunctionsTest.java shows, that adding a list of temperatures based on °C results in 1 °C + 1 K = -271.15 °C, using K we get 1 °C + 1 K = 275.15 K.

The third test method testSumTemperatureK2C confirms, that a sum in K converted back to °C results in 1 °C + 1 K = 2 °C. So both Indriya and uom-se have the necessary function for that already. Guess I'll change my vote from C to D.

desruisseaux commented 6 years ago

Adding temperature makes sense in thermodynamics: this is equivalent to adding kinetic energy (scaled by Boltzmann constant factor). Furthermore the problem is not only about addition, but also multiplication. What is the result of 2 * 10°C? This web site said "The common belief, that twice 10°C (or °F) equals 20°C (or °F), is wrong". Actually it depends if 10°C is an absolute temperature or an increment.

If we adopt a convention, then there is the consequences:

I don't think there is a safe, consistent, non-ambiguous way to perform the correct calculation without knowing if the value is an absolute measurement or an increment. It can be a Quantity property, a Unit property or a Quantity subtype. Jean-Marie's proposal to make it a Unit property instead of Quantity seems good to me.

desruisseaux commented 6 years ago

@keilw the fact that testSumTemperatureK2C verifies that 1°C + 1 K = 2°C does not mean that this is the right thing to do. As explained above, it depends on what 1°C and 1 K are. If we don't specify that, you will always find users saying that 1°C + 1 K = 2°C is wrong.

keilw commented 6 years ago

Based on the original ticket in SmartHome it seems, the problem @kaikreuzer mentioned there can be solved. Even with the old uom-se implementation.

@desruisseaux @dautelle Can you explain what

Jean-Marie's proposal to make it a Unit property instead of Quantity seems good to me.

Should look like?

dautelle commented 6 years ago

As always there is no definite "right" answer to the problem. But there are somes properties which have to be respected:

  1. Temp.a + Temp.b = Temp.b + Temp.a (commutativity)
  2. Temp.a.inAnyUnit + Temp.b.inAnyUnit = Temp.a.inAnyOtherUnit + Temp.b.inAnyOtherUnit

Since we add quantities, the unit should be irrelevant. The real question is: What quantity is 1°C ? Is it equal to 1 °K or 274,15 °K ? If you consider that it is 1°K then a statements such as "the temperature is 20°C today" is wrong (unless you are located on Pluto surface ;) If you consider that it is 274,15 °K then a statements such as "the temperature tomorrow is 5°C greater than today" is wrong (we are not on Mercury surface either ;) What ever the choice, there will be confusion. What is the least objectionable solution? I believe it is the second one which corresponds better to the physical quantities (kinetic movement). The statement "the temperature tomorrow is 5°C greater than today" will be incorrect, the correct statement will be "the temperature tomorrow is 5°K greater than today". In other words when dealing with delta/differences the correct unit should always be °K

keilw commented 6 years ago

JSR 363 is Final for almost 2 years and was available as Draft about 1 1/2 years longer, and I don't remember a single issue ticket saying "1°C + 1 K = 2°C is wrong, please fix it".
Actually until the SmartHome question we never even received an issue saying "1 °C + 1 K = -271.15 °C is problematic for my use case, please fix it".

So not sure, if we shall add complexity to the API and cause all sorts of headaches when dealing with "Compound" formatting (if we even do in 2.0?) for rare corner cases.

Allowing to control which target unit is used for a particular operation can solve the "1 °C + 1 K vs. 1 K + 1 °C" dilemma.

desruisseaux commented 6 years ago

Possible approach: add the following methods in Unit interface:

Then, update Unit.getConverterTo(Unit) contract as below:

For implementation of Quantity arithmetic operations, if the right converter is used, correct calculation should follow automatically. Example (omitting parameterized types for simplicity):

Quantity add(Quantity that) {
    Unit u1 = this.getUnit();
    Unit u2 = that.getUnit();
    Unit uc = u1.level(u2.getLevel());    // We will convert in unit of u1, but taking in account the nature of u2 (quantity or increment).
    UnitConverter c = u2.getConverterTo(uc);
    newValue = this.getValue() + c.convert(that.getValue());

    // The result is in unit of u1, but is it an absolute value or an increment?
    // Note: following code could be factorized in a convenience method.
    boolean isIncrement1 = u1.getLevel() == UnitOfMeasurement.INTERVAL;
    boolean isIncrement2 = u2.getLevel() == UnitOfMeasurement.INTERVAL;
    boolean isResultAnIncrement = u1 & u2;
    Unit uf = u1.level(isResultAnIncrement ? UnitOfMeasurement.INTERVAL
                                           : UnitOfMeasurement.RATIO);

    return new Quantity(newValue, uf);
}

Yes, I agree it looks complicated. But actually I think it only adds one line of code in arithmetic method implementations if the code computing uf is factorized in a convenience method. If we don't do that, then the complexity will be in the Javadoc instead than in the code since we will have to specify a behavior based on conventions, which I think may be worst.

desruisseaux commented 6 years ago

@keilw this whole thread started from a bug report in adding temperature values. Current behavior is mathematically inconsistent, or at least ambiguous. "1°C + 1 K" must be equal to "1 K + 1°C" - unless we throw away fundamental mathematical properties like commutativity. And what should be "1°C + 1°F"?

We can not escape this complexity - either we augment the API, or either we put the complexity in the Javadoc. I think the later is more dangerous because the users would have to remember a set of arbitrary conventions when using the Unit API.

I think that this issue should have priority over compound units because it touches mathematical properties, while compound units (as I see it - I may be wrong) seems mostly a formatting issue.