search

unitsofmeasurement / indriya

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

BigDecimal vs Double: Conversion with very small values to another unit gives a different result. #391

Closed wautergu closed 1 year ago

wautergu commented 1 year ago

We found a possible bug: We created 2 Quantities, both with the same small value, but the value of the first Quantity is constructed with a BigDecimal and the second one with a Double. Units are for both Quantities the same. When we convert both Quantities to another Unit, we get different results for the converted value.

The behaviour is written down in a test: possible_bug.tar.gz

keilw commented 1 year ago

Not sure, how you compare them, but it's not a bug. equals() just is very strict, see the same for BigDecimal and Double itself: BigDecimal.ONE.equals(new Double(1)) returns false.

So the two Number types are quite likely never the same if you mix BigDecimal and Double.

Please use isEquivalentTo() or compareTo() in AbstractQuantity which also deviates from equals() much like what was done inside the JDK BigDecimal class.

As for the numerical value itself, will have a look what causes that.

pascalriquier commented 1 year ago

Yeah, our issue is indeed that constructing the quantity with a BigDecimal leads to a negative value. We are aware that the equals of BigDecimal is strict, that is not our problem. We added the example with the double value to show that does work correctly ...

wautergu commented 1 year ago

Sorry, my assertion in the test is indeed wrong.

It looks like an overflow happened when the BigInteger with value 86400000000000000000 is converted to long in the multiplyWideAndNarrow function in the DefaultNumberSystem class on line 769 ( wide is a BigDecimal, narrow a BigInteger).

keilw commented 1 year ago

It looks like there's a misconception that that 'narrow' is one of {(Atomic)Long, (Atomic)Integer, Short, Byte, but BigInteger gets overlooked or subsummarized into the other mentioned types.

By checking for BigInteger, it should be possible to do new BigDecimal(narrow) instead of using the longValue().

keilw commented 1 year ago

Fixed with the upcoming MR2 of JSR 385. Please try using Indriya 2.2-SNAPSHOT, it fixes the attached testcase.