ionspin / kotlin-multiplatform-bignum

A Kotlin multiplatform library for arbitrary precision arithmetics
Apache License 2.0
362 stars 41 forks source link

toPlainString broken in 0.3.7 #243

Closed sproctor closed 2 years ago

sproctor commented 2 years ago

Describe the bug toPlainString is off by a few orders of magnitude in 0.3.7

To Reproduce I'll just post my tests here because I haven't investigated this too deeply.

assertEquals("100", BigDecimal.fromInt(100).round(BigDecimal.ONE).toPlainString())

fun BigDecimal.round(interval: BigDecimal?): BigDecimal {
    return if (interval != null && !interval.isZero()) {
        divide(interval, DecimalMode(scale = 0, roundingMode = RoundingMode.ROUND_HALF_AWAY_FROM_ZERO))
            .multiply(interval)
    } else {
        this
    }
}

resutls:

expected:<[10]0> but was:<[]0>
Expected :100
Actual   :0

Expected behavior Same output as 0.3.6

Platform

ionspin commented 2 years ago

Hi @sproctor , thanks for reporting, I've looked into it, and it looks like everything is working as expected. There were some bugs in 0.3.6 regarding the division that made your code work correctly, but now that they are fixed your round function is broken. When you create a decimal mode the way you do, the decimalPrecision parameter is 0 and that causes divide function to return 0 because no precision was required.

You would need to add the precision you require alongside the scale.

Ans yes, I know that the decimal precision API is AWFUL at the moment, I will be working on completely removing decimal precision as a parameter and working only with scale going forward.

Anyways, this change would work, in which I am setting the decimalPrecision parameter to current precision of the BigDecimal that you are rounding because the expected final precision will be either same or smaller:

fun BigDecimal.round(interval: BigDecimal?): BigDecimal {
            return if (interval != null && !interval.isZero()) {
                divide(interval, DecimalMode(decimalPrecision = precision, scale = 0, roundingMode = RoundingMode.ROUND_HALF_AWAY_FROM_ZERO))
                    .multiply(interval)
            } else {
                this
            }
        }
sproctor commented 2 years ago

Sorry for misreporting this issue. toPlainString is my way of getting information out of the API. From my point of view, I had a bunch of code that was working in Java, I ported it to KMP BigNum, it worked, then 0.3.7 broke it. I'm trying to understand what the difference between precision and scale is. The change in 0.3.7 broke all of my conversions. According to the docs 0 precision means unlimited. https://github.com/ionspin/kotlin-multiplatform-bignum/blob/main/bignum/src/commonMain/kotlin/com/ionspin/kotlin/bignum/decimal/DecimalMode.kt#L76

I'm trying to constrain the number of digits to the right of the decimal point, so it seems like using 0 for the precision is correct.

ionspin commented 2 years ago

No worries, as I mentioned decimal precision was a bad idea from the start, but here is an explanation BigDecimal is just a BigInteger (significand) with a certain exponent,

i.e. significand 34567 and exponent is 2 -> Big integer 3.4567 * 10^2 = 345.67 This number automatically has a precision of 5 because that is how many digits it's significand have.

Heres an example

@Test
    fun showBigDecimal() {
        val bigDecimal = "345.67".toBigDecimal()
        println("Significand: ${bigDecimal.significand}")
        println("Exponent: ${bigDecimal.exponent}")
        println("Precision ${bigDecimal.precision}")
        println("Is using scale? ${bigDecimal.usingScale} Scale ${bigDecimal.scale}")
    }

Result:
Significand: 34567
Exponent: 2
Precision 5
Is using scale? false Scale -1

If you were to use custom decimal mode with a higher precision that would just make the significand larger, i.e. precision 7 would make significand 3456700 but it would still represent the same big decimal. (There is a bug I found just now that discards the decimal mode in this case https://github.com/ionspin/kotlin-multiplatform-bignum/issues/244)

And just a bit of history interjection, scale didn't exist in this library from the beginning, it was a contribution down the line.

Scale on the other hand represents the number of digits to the right of the radix, and you have to specify it in your decimal mode.

Here is an example of scale increasing the precision of the number

@Test
    fun showBigDecimalWithHigherPrecision() {
        val bigDecimal = "345.67".toBigDecimal(decimalMode = DecimalMode(scale = 5, roundingMode = RoundingMode.CEILING)) 
        println("Significand: ${bigDecimal.significand}")
        println("Exponent: ${bigDecimal.exponent}")
        println("Precision ${bigDecimal.precision}")
        println("Is using scale? ${bigDecimal.usingScale} Scale ${bigDecimal.scale}")
        println("DecimalMode ${bigDecimal.decimalMode}")
    }

Result:
Significand: 34567000
Exponent: 2
Precision 8
Is using scale? true Scale 5
DecimalMode DecimalMode(decimalPrecision=8, roundingMode=CEILING, scale=5)

So to sum it up precision is total number of digits in the significand, the scale is number of digits to the right of the radix. I know it's confusing especially when you need to define both precision and scale in some cases, like yours, and I am going to rewrite it to remove decimalPrecision parameter completely, but it's a lot of work and I am very limited on free time at the moment.

Hope this helps you understand it a bit better.

And just the last note, while this library is inspired partially by the Java Big Integer, it's not a one to one re-implementation.