unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
118 stars 40 forks source link

Quantity Multiplication might produce counterintuitive Results #294

Closed andi-huber closed 4 years ago

andi-huber commented 4 years ago

Since support for special quantities that have a distinction between relative and absolute scale, multiplication changed behavior generally, such that the multiplication result has its units converted to system units.

We need to restore the previous behavior, that is, don't change units, instead carry them over to the result. (At least as long this is possible. Eg. shifted unit operands do require conversion to system units before multiplication.)

keilw commented 4 years ago

I'll check it out later today, thanks for the quick resolution.

keilw commented 4 years ago

What exactly was fixed? I ran the CO2CarDemo and it still tells me the "Ferrari-Enzo6" produces 1.03398386000 kg of CO2, but it should be 1.033 tons.

andi-huber commented 4 years ago

Should produce the correct result if run against latest Indriya from 'master'. Can you try and verify?

andi-huber commented 4 years ago

(fingers crossed)

keilw commented 4 years ago

No sorry I had a tweak where I explicitly converted to the system unit in CO2CarDemo, once that's removed it produces the expected tons again.

keilw commented 4 years ago

IC you created a SpecificCarbonEmission, will factor that into the next snapshot of uom-domain-energy-quantity.

andi-huber commented 4 years ago

Yep, go for it.

keilw commented 4 years ago

I plan to release 2.0.4 soon, probably by Friday (the 24th ;-) WDYT about the TCK, should there be a minor improvement like taking the SCALE into consideration somewhere? Also to @desruisseaux, currently we only have testQuantityGetScale() to ensure, compatible implementations override/implement that method, but there are no assumptions e.g. that certain operations shall result in a system unit. If you think, a TCK 2.0.1 was justified, please create tickets in https://github.com/unitsofmeasurement/unit-tck/issues.

andi-huber commented 4 years ago

To be honest, I was struggling with the arithmetic implementation for multiplying quantities taking the SCALE into consideration. We have worked out a guildine for the rules https://github.com/unitsofmeasurement/unit-api/wiki/Arithmetic-rules-for-Difference-versus-Absolute-quantities, but that's not a 100% what I implemented.

andi-huber commented 4 years ago

So what I did: Any multiplication of quantities regardless of SCALE will convert its 2 operand quantities before executing the (numerical) multiplication operation to be ABSOLUTE and also to be LINEAR (in the sense as we defined in the API), which will yield an ABSOLUTE result always and means conversion to system units for one or both of the operands sometimes.

For almost all units, conversion from RELATIVE to ABSOLUTE has only semantic, but no numeric consequences. However, taking eg. CELSIUS, the numerical conversion algorithm we pick depends on the quantity's scale. But regardless of the CELSIUS operand's SCALE, we always convert this one to system units (KELVIN) such that the operand is given in LINEAR units.

Thats the only way I can make sense of such multiplication results. I might be wrong and we can try to implement this differently. But it seems none of our existing JUnit Tests contradicts this approach.

andi-huber commented 4 years ago

Regarding TCK, maybe we should regard the SCALE topic still somewhat experimental

keilw commented 4 years ago

JUnit tests that would break should also break the build, but the question is, if any additional TCK test in https://github.com/unitsofmeasurement/unit-tck would make sense for that? I am not entirely sure, if we could check that in the TCK, maybe for cases where it is converted to a system unit we might try something in the TCK if it's generic enough and not too restrictive to implementations.