Bug 66047 - Rounding issue in MROUND due to 1897066
Summary: Rounding issue in MROUND due to 1897066
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 5.2.1-FINAL
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-29 12:10 UTC by Fabio
Modified: 2022-05-03 13:53 UTC (History)
0 users



Attachments
MRound BigDecimal implementation (2.62 KB, patch)
2022-04-29 12:10 UTC, Fabio
Details | Diff
Excel File with rounding issue (8.67 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2022-04-29 12:11 UTC, Fabio
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Fabio 2022-04-29 12:10:43 UTC
Created attachment 38271 [details]
MRound BigDecimal implementation

Hi

I noticed a rounding difference after upgrading to poi and tracked the change back to version 5.2.1 https://svn.apache.org/viewvc?view=revision&revision=1897066.

I do acknowledge the new calculation as more correct, but it creates a follow up problem when using the MROUND formula.

Consider the following example: MROUND(0.79*7.5;0.05).
In Excel, the result is 5.95. In POI 5.2.1 the result ist 5.90.

This happens because earlier POI version evaluated 0.79*7.5 to 5.925000000000001. The new BigDecimal based calculation ist instead (correctly) evaluating this to 5.925.

The MRound implementation is rounding this down, where as before it was rounding up:
> result = multiple * Math.round( number / multiple );

In my opinion, it would make sense to replace this implementation with a BigDecimal version and the HALF_UP rounding mode (see patch).

Thanks!
Comment 1 Fabio 2022-04-29 12:11:18 UTC
Created attachment 38272 [details]
Excel File with rounding issue
Comment 2 PJ Fanning 2022-04-29 12:26:09 UTC
thanks - patch applied with r1900376
Comment 3 Fabio 2022-05-02 10:14:28 UTC
Hi

I have second thoughts about the rounding improvements in https://svn.apache.org/viewvc?view=revision&revision=1897066.

My patch does solve some cases, but it also does something different than the excel implementation. With my patch, the example of MS' "Known Limitations" breaks. Both, 6.05 and 7.05 are now rounded deterministically to 6.1 respectively 7.1. The example from Microsoft rounds to 6.0 and 7.1.

In my opinion, consistent rounding is definitely better, but then the POI implementation is not consistent with Excel :-(
Finding quirks to accommodate for Excel's undefined behaviour is error prone and unmaintainable.

I don't know what's the best way forward here. Reverting 1897066 is probably not an option either, is it?

[1] https://support.microsoft.com/en-us/office/mround-function-c299c3b0-15a5-426d-aa4b-d2d5b3baf427
Comment 4 Axel Howind 2022-05-02 12:31:55 UTC
I am not sure if this is really more consistent. As I have said in a comment to another issue, there is no exact 6.05 and no exact 7.05 in excel as excel internally uses IEEE754 numbers. Both numbers are represented by their nearest repesentable neighbour, and the fractional part of both representations ($1 and $2) is < 0.05, so a consistent solution would round both numbers down. What happens here is that the errors in the floating point representations of 7.05 ($2) and 0.1 ($3) cancel each other out and lead to the result 7.1 ($5 rounded to one decimal).

You can check this in jshell by using the double constructor of BigDecimal (note that you should avoid that constructor at all costs for other purposes):


```
    % jshell 
    |  Welcome to JShell -- Version 18.0.1
    |  For an introduction type: /help intro
    
    jshell> new BigDecimal(6.05)
    $1 ==> 6.04999999999999982236431605997495353221893310546875
    
    jshell> new BigDecimal(7.05)
    $2 ==> 7.04999999999999982236431605997495353221893310546875
    
    jshell> new BigDecimal(0.1)
    $3 ==> 0.1000000000000000055511151231257827021181583404541015625
    
    jshell> new BigDecimal(6.05*0.1)
    $4 ==> 0.604999999999999982236431605997495353221893310546875
    
    jshell> new BigDecimal(7.05*0.1)
    $5 ==> 0.7050000000000000710542735760100185871124267578125
```

This is the reason why you get 6.0 and 7.1 respectively in Excel.
Comment 5 PJ Fanning 2022-05-02 18:56:06 UTC
I'm currently travelling so can't devote much time at the moment for this. I would prefer not to revert the previous change. In practice, getting POI code in Java to act like Excel does is 100% of cases is a real pain and we should drive any solutions by using test cases based on observed behaviour in Excel.

Your best solution is to set wb.setForceFormulaRecalculation(true); - so that Excel will recalc all the values when it loads the workbook.
Comment 6 Yegor Kozlov 2022-05-03 13:53:17 UTC
I'd say POI is good and on par with other implementations. Google Sheets and LibreOffice consistently evaluate MROUND(6.05,0.1) to 6.1 and MROUND(7.05,0.1) to 7.1.