search

unitsofmeasurement / indriya

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

Incorrect addition in LongQuantity when conversion to fractions is required #219

Closed keilw closed 5 years ago

keilw commented 5 years ago

While add() works for NumberQuantity or DecimalQuantity even if conversions to a fraction is required, there seems to be a rounding problem in LongQuantity.

While quantityConstruction() in MixedRadixTest results in DecimalQuantity or NumberQuantity values with the correct result of "1 ft 2 in" being 1.1666666666666667 ft or 14 in, the parsed result in mixedParsing() of CompoundQuantityFormatTest uses two LongQuantity values, where add() seems to drop an inch because 13.999999999999999888 in end up as 13 in after castFromBigDecimal().

keilw commented 5 years ago

@filipvanlaenen (or others e.g. @andi-huber) do you have an idea how to address this in LongQuantity.castFromBigDecimal() without breaking the test LongQuantityTest.additionWithLargerMultipleAndOverflowingResultCastsToLargerMultiple()?

andi-huber commented 5 years ago

When doing a code review lately, I found the idea of having Quantity implementations for every concrete Number type in Java an error prone approach. I was investigating a different solution to this idea, that is to simply implement a Calculator class, that knows how to best add, subtract, multiply, divide and 'integer divide with remainder' any arbitrary java Numbers thrown at it.

We would then encapsulate all the arithmetic complexity regarding different Number types within this calculator class. Junit-testing such a calculator seams pretty straight forward. Once we had such a calculator, we could then use it throughout RI, where ever number arithmetic is required.

It would require some refactoring and we would also need to let go of these JavaNumericQuantity implementations. If you like this refactoring idea, I can offer to work on such a Calculator.

keilw commented 5 years ago

We should be careful when touching NumberQuantity which for many cases is also a facade via of() but JavaNumericQuantity and all concrete subclasses are package-private and hidden from the outside, so as long as JUnit tests don't break, please go ahead. It should be time-boxed and not take longer than about a week from now. We should be in a position to submit the Public Draft for review around May 1. I fixed a few things in the RI, but for now, I think you're good to refactor and create PRs. I cleaned up the relation between MixedRadix and CompoundQuantity (given http://www.thefreedictionary.com/Compound+quantity calls it so, I could easily live with the name) I closed https://github.com/unitsofmeasurement/indriya/issues/200 because it applied to an earlier approach, if you see a need for further improvement of MixedRadix, feel free to create a ticket, but I would not force that into the 2.0 release of Indriya yet, it can always be part of improvement updates in service releases.