ionspin / kotlin-multiplatform-bignum

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

Rounding Issue #71

Closed avjiang closed 4 years ago

avjiang commented 4 years ago

Hi @ionspin !

I am having problem with figure rounding. I have an application that can support trailing decimal point number configuration. We have a setting that will configure this thing, so it will be dynamic.

Let's say the total decimal point number set is 3. So given number 12.5671, it will round to 12.567, if it is 2, it becomes 12.56 and so on.

I have a test case like following

image

and here is my round function

image

The purpose why I convert to BigInteger first is to get the number before the decimal point, and then use the value from the setting to add the trailing decimal point. The test case above will result to java.lang.reflect.InvocationTargetException

Can you guide me on how to directly use the BigDecimal number and then use my custom setting value to set the trailing decimal point number?

If it is still unclear to you, please let me know, so I can explain more to you. Thanks for this amazing library!

ionspin commented 4 years ago

Hey, I can definitely help you with that, and you should be able to do everything by using BigDecimal and decimal mode directly, I'll just need some time to write an understandable explanation, because it's a bit different from the way Java BigInteger does it. Could you in the mean time grab a stack trace when that InvocationTargetException is thrown, because that is definitely a bug.

Thanks!

ionspin commented 4 years ago

Ok, so while going through the process, I found a couple of bugs, so I'd suggest that you use a snapshot until I release a next version.

Your approach will give you a correct result, but it's creating an uneccessary BigInteger instance, and the rounding mode is also giving you troubles, so let's look at a different way of getting to the desired result.

So to round to a specific number of digits after the decimal point, you should be looking at two properties of BigDecimal, the precision and exponent

precision - describes how many digits does the significand have `exponent - exponent describes to which degree the significand needs to be raised/lowered to properly describe the number.

One important thing to keep in mind is that significand (which is a BigInteger) is NOT representing an integer! I know it's a bit confusing, but keeping that in mind will help you when debuggin later.

Some examples will make it more clear (pay attention to where the decimal point is when calculating the big decimal):

Significand = 123
Exponent = 3
BigDecimal = 1.23 * 10^3 = 1230
Significand = 654
Exponent = -3
BigDecimal = 6.54 * 10^-3 = 0.00654
Significand = 987
Exponent = 0
BigDecimal = 9.87 * 10^0 = 9.87

Hope that clears it up a bit.

And that would be enough and work perfectly, if we had unlimited memory, but because we don't we need to introduce desired precision parameter. Otherwise cases such as 10/3 =0.3333... would result in all the memory being used to store the repeating 3.

When you are creating BigDecimals without specifying DecimalMode they default to infinite precision (it's represented with precision = 0, and rounding mode NONE)

In those cases, precision will always equal the length of underlying significand.

So let's look at how BigDecimal can look if we introduce precision:

Lets use this as our BigDecimal BigDecimal.fromIntWithExponent(123456789, 3)

what we will actually get is the following

Significand = 123456789
Precision = 9
Exponent = 3
BigDecimal = 1.23456789 * 10^3 = 1234.56789

So if you want to have the BigDecimal with a specific number of digits after the decimal point, you should be modifying the precision parameter.

So let's say that we want to convert that BigDecimal from previous example into a BigDecimal with just 2 trailing digits.

We would first look if the exponent is positive, because if it is, we know that the position of the decimal point is at position exponent + 1. So in our case that will be 4.

If the exponent is negative, it gets a little trickier, but you'll figure it out if you play around with it a bit.

Then we want the new precision to be 4 + 2 (desired number of digits after the decimal point), so 6.

And this far your rounding method would work fine, but the problem is the RoundingMode cannot be NONE, you need to pick a RoundingMode that suits your calculation, you can look at different rounding modes and their description on wikipedia article on Rounding

So doing something like BigDecimal.fromIntWithExponent(123456789, 3).round(DecimalMode(6, RoundingMode.ROUND_HALF_AWAY_FROM_ZERO))

That should give you the desired result of 1234.57 (note the 7 at the end because of rounding)

There is another caveat here. This rounded number now has a set decimal mode of 6 and HALF_AWAY_FROM_ZERO

Next time you use this number in a arithmetic operation, and you don't specify desired decimal mode for the operation yourself, it will use the one that came with the number. Which might not give you desired result if the second number has a precision higher than 6.

I haven't had the time to tackle that yet, but I will probably add a global BigDecimal DecimalMode parameter where you can set the default overrideing DecimalMode for all operations, that would then discard the DecimalMode that comes with the number itself. But this might be an API breaking change, so be warned.

Hope this clears up things a bit, if you have a suggestion on how you would LIKE this to work, feel free to share it, as the library should be easy to use, and this is the perfect time to break the API in early stages.

I'll also add a method called something like `roundToDigitsAfterRadix(numberOfDigitsAfterRadix : Int) that will do exactly what you want.

ionspin commented 4 years ago

Hey @avjiang I added a couple of methods that should make this much more straightforward. roundToDigitPosition and roundToDigitPositionAfterDecimalPoint, they are in snapshot so you can try them out. I still have to do some cleanup and add documentation.

Also you'll notice that I broke the previous API by renaming round() to roundSignificand() which describes better what it actually does, I'm not completely sure that I will leave it public at all with those two new methods. Anyways, that change is going to break your build, so sorry about that :)

avjiang commented 4 years ago

hi @ionspin, these few days my team still working on it. Thank you so much for the detailed explanation! I think I can try those 2 new methods first, see how it works. Will come back to you again later :)

avjiang commented 4 years ago

I have tried with all three rounding methods provided. My objective is to round to 2 decimal points (after decimal point)

So these are the results tested with different rounding methods: roundSignificant: if(bigDecimal.exponent>0) bigDecimal = bigDecimal.roundSignificand(decimalMode = DecimalMode(bigDecimal.exponent.longValue() + 1 + 2, RoundingMode.ROUND_HALF_CEILING)) else bigDecimal = bigDecimal.roundSignificand(decimalMode = DecimalMode(-1 + 2, RoundingMode.ROUND_HALF_CEILING))

input:0.00123121 expected result:0.00 output:0.002

input:0.0123121 expected:0.01 output:0.02

input:0.1123121 expected:0.11 output:0.2

roundToDigitPositionAfterDecimalPoint var bigDecimal = input.roundToDigitPositionAfterDecimalPoint((input.exponent.longValue() + 2), RoundingMode.ROUND_HALF_TOWARDS_ZERO)

input:10.3333 expected: 10.33 result:10.34

input:10.345364 expected:10.35 result: 10.35

var bigDecimal = input.roundToDigitPosition((input.exponent.longValue() + 1 + 2), RoundingMode.ROUND_HALF_AWAY_FROM_ZERO) input:10.12345 expected:10.12 result:10.13

if i set the digit precision to 3 input:0.00123432432 expected:0.00 result:0.01

I have tested with ROUND_HALF_CEILING and ROUND_HALF_AWAY_FROM_ZERO rounding mode, but it does not return me the result as expected. I would like to round figure below 5 to 0 and figure 5 and above to next number

ionspin commented 4 years ago

Ah, I'll have to look into the rounding modes, I'll try to do that today after work. ROUND_HALF_AWAY_FROM_ZERO should be the one to use, but it's not giving proper results in your testing. Let me get back to you on this.

ionspin commented 4 years ago

I've found a bug in the rounding function, it would return wrong results most of the time, but unfortunately my tests covered only working cases. I also changed the two new funcations a bit. Here's how it works now:

        assertTrue {
            val rounded = BigDecimal.parseString("1000000.12355")
                .roundToDigitPositionAfterDecimalPoint(3, RoundingMode.ROUND_HALF_AWAY_FROM_ZERO)
            rounded.toStringExpanded() == "1000000.124"
        }

        assertTrue {
            val rounded = BigDecimal.parseString("1000000.12355")
                .roundToDigitPositionAfterDecimalPoint(3, RoundingMode.ROUND_HALF_TOWARDS_ZERO)
            rounded.toStringExpanded() == "1000000.123"
        }
ionspin commented 4 years ago

It should be available in the snapshot in an hour or so, once the Travis build is complete. And a heads up tomorrow or a day after, I will be changing exponent in big decimal from BigInteger to Long so you won't have to do this ... input.exponent.longValue() ... any more. Will break the build again :)

avjiang commented 4 years ago

I tried to run your test again, but it failed. i am using version 0.1.4-SNAPSHOT

ionspin commented 4 years ago

Hi @avjiang, I presume that snapshot didn't update on your side, gradle can be sometimes finicky if you don't force snapshot update. In any case, version 0.1.4 was just fully released so you might want to try with that one.

avjiang commented 4 years ago

Hi @ionspin, updated to version 0.1.4, and I found another issue

image

Expected result is: 1000000.12 However, the function will return 1000000.13

Please advise

ionspin commented 4 years ago

Excellent catch, it's because the discarded part is "055" which is unfortunately inspected as "55". I'll fix that after work and put the fix in 1.5-SNAPSHOT.

ionspin commented 4 years ago

@avjiang Fixed in 0.1.5-SNAPSHOT, but I didn't have time to test properly, so I wouldn't be surprised if you find some other corner cases. Thanks for reporting the problems and providing the test cases, it helps a lot!

ionspin commented 4 years ago

I'm considering this issue resolved as #77 should have taken care of final problems. If there are further rounding problems we should track them in new issues.