unitsofmeasurement / indriya

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

Converting the unit of relative quantities always yields absolute quantities, round trip with temperatures fails #295

Closed ledan87 closed 4 years ago

ledan87 commented 4 years ago

I stumpled upon an issue when writing api tests for indriya 2.0.3:

Consider the following (testng) unit test:

...
import static org.testng.Assert.*;
import static tech.units.indriya.unit.Units.*;
...

@Test
public void convertRelativeTemperaturesRoundTrip() {

      Quantity<Temperature> celsiusRelative = Quantities.getQuantity(BigDecimal.valueOf(1), CELSIUS, Quantity.Scale.RELATIVE);
      Quantity<Temperature> kelvinRelative = celsiusRelative.to(KELVIN);

     // this test is already failing, as the Scale is absolute
      assertEquals(kelvinRelative, Quantities.getQuantity(BigDecimal.valueOf(1), KELVIN, Quantity.Scale.RELATIVE));

      Quantity<Temperature> roundTrip = kelvinRelative.to(CELSIUS);
      // this one fails as well, as the quantity of roundTrip is now -272.15 °C
      assertEquals(celsiusRelative, roundTrip);

}

It seems that the wrong factory method is called in AbstractQuantity.to(Unit)

Cheers, Daniel

keilw commented 4 years ago

This could be related to other calculation issues and peculiarities we stumbled over after the 2.0.3 changes. @andi-huber would you also have a look at this please together with #294? Thanks, Werner

andi-huber commented 4 years ago

3 observations regarding these tests: 1) The scope information is held by Quantities not Units. 2) For Quantities of KELVIN there is no distinction between RELATIVE and ABSOLUTE, they are the same. So I think KELVIN RELATIVE is always converted to KELVIN ABSOLUTE implicitly. 3) The conversion of a Quantity to a different Unit will always implicitly convert to ABSOLUTE scope.

andi-huber commented 4 years ago

That said, the tests above seem intuitively correct to me and maybe we can improve the library such that these do work,

keilw commented 4 years ago

What's the "scope", do you mean "scale"?

andi-huber commented 4 years ago

ah sorry, yes scale

ledan87 commented 4 years ago

That said, the tests above seem intuitively correct to me and maybe we can improve the library such that these do work,

I think that only requires calling the right factory method for Quantities in AbstractQuantity.to(Unit unit) => line 207.

I just rolled back from 2.03. to 2.0 and checked the conversion for T_rel = 1 °C to KELVIN. It yielded 274.15 K, which is also not correct.

In 2.0.3 the result for the conversion is 1 °C => 1 K, but the result is absolute and this is leading to errors downstream. When converting the units of relative quantities, the result should always be a relative quantity.

andi-huber commented 4 years ago

@ledan87 thanks for spotting this. I have supposedly fixed this one. If you find new issues, don't hesitate to let us know.