ionspin / kotlin-multiplatform-bignum

A Kotlin multiplatform library for arbitrary precision arithmetics
Apache License 2.0
364 stars 42 forks source link

Multiply looses precission #176

Closed lhoracek closed 3 years ago

lhoracek commented 3 years ago

Describe the bug Numbers rounded to specific number of of digits after decimal point multiply incorrectly.

To Reproduce Steps to reproduce the behavior:

val a = "5.61".toBigDecimal().roundToDigitPositionAfterDecimalPoint( 2, RoundingMode.ROUND_HALF_AWAY_FROM_ZERO)
val b = "2.95".toBigDecimal().roundToDigitPositionAfterDecimalPoint( 2, RoundingMode.ROUND_HALF_AWAY_FROM_ZERO)
val res = a.multiply(b).roundToDigitPositionAfterDecimalPoint( 2, RoundingMode.ROUND_HALF_AWAY_FROM_ZERO)
assertEquals("16.55", res.toStringExpanded())
expected:<16.5[5]> but was:<16.5[]>
Expected :16.55
Actual   :16.5

Expected behavior

Result should not loose precision same way java BigDecimal.setScale(2, RoundMode.HALF_UP)

BigDecimal bd1 = new BigDecimal("5.61").setScale(2, RoundingMode.HALF_UP); 
BigDecimal bd2  = new BigDecimal("2.95").setScale(2, RoundingMode.HALF_UP); 
BigDecimal bs3 = bd2.multiply(bd1).setScale(2, RoundingMode.HALF_UP); 
String str = "Result is " +bs3;
System.out.println(str); 

Platform JVM (commonTest run by testDebugUnitTest)

ionspin commented 3 years ago

Multiplication didn't loose precision, roundToDigitPositionAfterDecimalPoint was setting a DecimalMode to the number of digits in the result after rounding which then caused multiplication to round to that decimal mode, which in this case was 3. Still I feel this was completely unexpected behavior of roundToDigitPosition methods, so now #177 will change the behavior so they only apply decimal mode after rounding if the users sets it explicitly.

ionspin commented 3 years ago

Oh btw, the fix will be available in 0.3.1-SNAPSHOT as soon as the gitlab build is completed.

ionspin commented 3 years ago

And also, thanks for reporting!

lhoracek commented 3 years ago

Will test ASAP, but even when I use val dm = DecimalMode(99999, RoundingMode.ROUND_HALF_AWAY_FROM_ZERO, 2) and create new BigDecimal with fun BigDecimal.round() = BigDecimal.fromBigDecimal(this, dm)

val out = ("5.61".toBigDecimal().round() * "2.95".toBigDecimal().round() * "14.41".toBigDecimal().round()).round()
assertEquals("238.48", out.toStringExpanded())
expected:<238.4[8]> but was:<238.4[9]>
Expected :238.48
Actual   :238.49

Are my expectations wrong here? The actual result is 238,478295 .. or is this cause by the actual overloaded operator, that does the rounding after each of those operations?

lhoracek commented 3 years ago

Or better question is "how to properly do it with this lib"? I have to admit I've expected just using setScale the same way I did with java's BigDecimal

ionspin commented 3 years ago

I am not completely sure, I would have to take step through debugger to see what is going on with your sample, but I don't have time right now. Keep in mind that the when using decimal mode with scale, scale overrides the decimal precision so even if you put 9999 the scale 2 is used. DecimalMode API will be changed at some point to be more user friendly, at the moment there are too many pitfalls.

ionspin commented 3 years ago

Or better question is "how to properly do it with this lib"? I have to admit I've expected just using setScale the same way I did with java's BigDecimal

Your usage DecimalMode(99999, RoundingMode.ROUND_HALF_AWAY_FROM_ZERO, 2) would work for scale 2. Again I'm not sure about the order of rounding you are applying without debugging it.

The original cause why this part of the API is not easy to use is because originally the library used only decimal precision, and scale was a later contribution. I'll probably be dropping the decimal precision from the API in the upcoming versions.

ionspin commented 3 years ago

From a quick test with the latest with the latest build that should be available in the snapshots this could work for you

        val dm = DecimalMode(99999, RoundingMode.ROUND_HALF_AWAY_FROM_ZERO, 2)
        fun BigDecimal.round() = BigDecimal.fromBigDecimal(this, dm)
        val out = ("5.61".toBigDecimal() * "2.95".toBigDecimal() * "14.41".toBigDecimal()).round()
        assertEquals("238.48", out.toStringExpanded())
lhoracek commented 3 years ago

Just tested it and it works great. Thanks for the quick fix!

lhoracek commented 3 years ago

Can be closed or left until new release is done. 👍