New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MathFlux sumBigDecimal looses scale of BigDecimal #260
Comments
That's a great catch @kiwisincebirth! After having looked into your suggestion to use the Would you like to submit a fixing PR for both sum and average? |
See The pull Request #261 for the change I proposed. I have updated all but one test to use verifyResult() instead of verifyBigDecimalResult() - It turns out the verifyBigDecimalResult() was actually masking the precision issue! Because verifyBigDecimalResult() uses compareTo() ==0 for its comparisons. Simply changing Tests to use verifyResult() - highlighted the stated issue. |
This commit ensures that no lossy conversion is made on a Number that happens to be a BigDecimal already. Furthermore, it uses the `new BigDecimal(String)` constructor instead of `valueOf`, since creating BigDecimal from doubles can sometimes result in less meaningful values due to double's limited precision. Fixes #260.
This commit improves the precision and conversion behavior of `MathFlux` `sumBigInteger` and `averageBigInteger` methods. These method better deal with fractional types by internally summing using a `BigDecimal` rather than a `BigInteger`. As a result, the only lossy step is at the end when the result is effectively presented as a `BigInteger`: - `sum`: remaining fractional part of the BigDecimal sum is dropped - `average`: the division by `count` is applied, then the result's fractional part is dropped Previously the fractional part would be progressively dropped by implicit conversion of each incoming element to `long`. Similar to precision improvement done in #260/#261 and earlier iteration done in #284.
MathFlux sumBigDecimal looses scale of BigDecimal. And is not respecting standard BigDecimal behaviour.
Expected Behavior
The summation in the Flux should follow BigDecimal arithmetic, using a standard plus() operator provided by BigDecimal.
Actual Behavior
The summation logic is loosing precision, and the cause is related to the conversion of BigDecimal to it Double value
Steps to Reproduce
The following test case (Kotlin code) highlights the error. I have not tried this in Java, but assume the same issue would be prevalent in Java Code also.
the result if the above is
Possible Solution
The issue seems to be in the Class MonoSumBigDecimal Line:77
where the Number being processed in the stream, is converted via Double back to a BigDecimal. For the general case of Number this is fine. but in this case the Number (IS) a bigdecimal and does not need to be cast in this way.
noting that number.toString() is my (OPINION) of how to convert a Number more generally into a BigDecimal, since the translation avoids the Double type.
I am fairly sure MonoAverageBigDecimal suffers from the same issue, since it has the same line of code indicated above.
Your Environment
Not Relevant the above test case fairly easy to reproduce
java -version
): 11The text was updated successfully, but these errors were encountered: