unitsofmeasurement / indriya

JSR 385 - Reference Implementation
Other
119 stars 42 forks source link

Conversion method sometimes returns with a BigDecimal value when executed on a Double value #312

Closed mia-krause closed 3 years ago

mia-krause commented 3 years ago

The following test is failing, even though I would expect it not to.

@Test
public void test() {
     ComparableQuantity<Length> q1 = Quantities.getQuantity(0.0001d, MetricPrefix.KILO(Units.METRE));
     ComparableQuantity<Length> convertedQ1 = q1.to(Units.METRE);
     ComparableQuantity<Length> q2 = Quantities.getQuantity(0.1d, Units.METRE);
     ComparableQuantity<Length> convertedQ2 = q2.to(Units.METRE);
     assertEquals(convertedQ1, convertedQ2);
}

I create q1 and q2 with a double quantity. Then I convert both quantities to metre, after which I would expect them to be equals. This isn't the case, because the value of convertedQ1 is now of type BigDecimal while convertedQ2 has a value of type Double. This happens because q2 does not need to be converted as it already has the desired unit, so it will be returned as is. q1 on the other hand needs to be converted. This conversion uses DefaultNumberSystem.multiplyWideAndNarrow(NumberType, Number, NumberType, Number), which wraps the numbers in BigDecimal for the multiplication, thus return BigDecimal as result.

This seems to be inconsistent behavior as sometimes if I am putting in a quantity with a double value and it returns one with a BigDecimal value, sometimes I do the same and get a double value.

andi-huber commented 3 years ago

assertEquals(convertedQ1, convertedQ2) does not guarantee to compare by value.

You might want to consider rewriting your equality check to

...
assertTrue(convertedQ1.isEquivalentTo(convertedQ2));

However, I strongly encourage you to use a more bullet proof approach, when comparing numbers for equality, that is, specify an epsilon that defines what precision you care about:

e.g with Indriya we use this class internally for testing number value equality: https://github.com/unitsofmeasurement/indriya/blob/233f00ff898c34f1e651b47dc5ee81560b135800/src/test/java/tech/units/indriya/NumberAssertions.java#L54

If that's overkill for your use-case, you might also just do quantity equality checks this way:

// check for equal units
assertEquals(convertedQ1.getUnit(), convertedQ2.getUnit()); 

// check for equal amounts within reasonable precision
assertEquals(convertedQ1.getValue().doubleValue(), convertedQ2.getValue().doubleValue(), 1E-6);
andi-huber commented 3 years ago

I think it would be nice to have some extensions to Quantity or ComparableQuantity that would allow to give a precision parameter delta for the equivalence check:

boolean isEquivalentTo(Quantity<Q> that, Number delta); // where delta automatically applies to the 'smaller' unit

Also to consider:

boolean isGreaterThan(Quantity<Q> that, Number delta);
boolean isGreaterThanOrEqualTo(Quantity<Q> that, Number delta);
boolean isLessThan(Quantity<Q> that, Number delta);
...

And maybe even deprecate the existing ones.

andi-huber commented 3 years ago

To answer the question that was implied: Why do Quantities change their number type on the fly?

That was a design decision made to allow Indriya v2 to do calculations with higher precision. It converts number types on the fly as needed. While this is convenient for most use-cases, it might be counter-intuitive, because the behavior was different in version 1.

eg. with v2 this is now possible

Quantity<Length> a = Quantities.getQuantity(1, Units.METRE);
Quantity<Length> b = Quantities.getQuantity(2, Units.METRE);

System.out.println(a.divide(b)); // --> prints 0.5

while the number that is internally used to represent to division result is not int and not double, but a custom RationalNumber type, which of course is a perfect representation of the number value 1/2 (not just a rounded one).

keilw commented 3 years ago

@andi-huber I'm not sure, if the "delta" really adds value rather than confusing people. If you do something like "1km equivalentTo 1000m" I would not want having to pass a delta every time, that seems an unnecessary burden. The main method isEquivalentTo() is already being promoted to the API since there are often cases where equals() just won't work, see Integer vs. BigInteger etc. all of which are not equal in Java. If there is enough value here for the delta options, I would keep them in ComparableQuantity for now. Unlike ComparableUnit which never had more than 2 or 3 methods, most of them convenience methods, it shall continue to exist, so if more methods are desired, why not create a feature enhancement ticket and we see, what the discussion with other contributors brings.

mia-krause commented 3 years ago

Thank you for the fast reply! isEquivalentTo doesn't work for us as we don't want 1km to be equals to 1000m, so we will go the second route and check unit and value individually. Thanks a lot!

keilw commented 3 years ago

@mia-krause Thanks for the reply. Do you need more help or could we close this? @andi-huber as mentioned, please for the overriden versions of isEquivalentTo() etc. feel free to create a new feature request and we'll discuss it there with everyone else like scalle and all the other features.

mia-krause commented 3 years ago

No, this has been helpful and can be closed, thanks!