Bug 138220 - calc: calculation: roundup: rounddown: fail in 7.1 alpha
Summary: calc: calculation: roundup: rounddown: fail in 7.1 alpha
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
(earliest affected) release
Hardware: All All
: medium normal
Assignee: Not Assigned
Keywords: bibisected, bisected, regression
Depends on:
Blocks: Cell-Formula
  Show dependency treegraph
Reported: 2020-11-14 20:52 UTC by b.
Modified: 2021-05-23 13:01 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:
Regression By:

test file (113.12 KB, application/vnd.oasis.opendocument.spreadsheet)
2020-12-01 16:34 UTC, zcrhonek

Note You need to log in before you can comment on or make changes to this bug.
Description b. 2020-11-14 20:52:45 UTC
a 'deflowering'? based on 'search' no bugs have been reported for 'roundup' and 'roundown' yet? 

i stumbled over one: 

=ROUNDUP(-999,12999999999994;11) results in -999,130000000010 instead of the expected -999,130000000000, 

=ROUNDDOWN(-999,12999999999994;11) results in -999,130000000000
instead of the expected -999.12999999999990

same fail - mirrored - for positive values, 

for those not familiar with it: roundup should round 'away from zero', rounddown 'towards zero', 

plenty other values affected as well, even shorter ones like '888,1299999999'
which need no change for a round to 11 decimals ... 

'round' looks working correctly on a quick first look, 

no repro in, hence 'regression', 

imho critical, data corruption ... 

Steps to Reproduce:
see above description

Actual Results:
see above description

Expected Results:
see above description

Reproducible: Always

User Profile Reset: No

Additional Info:
Version: (x64)
Build ID: b61bf7c7cfcf97a5ade6d130873af146670bc2ee
CPU threads: 8; OS: Windows 6.1 Service Pack 1 Build 7601; UI render: default; VCL: win
Locale: de-DE (de_DE); UI: en-US
Calc: CL

'unparallelized' (without openCL and threading) same effect,
Comment 1 b. 2020-11-15 19:33:10 UTC
above reported fail no repro in, but 

=ROUNDUP(8000000000000000;-15) results in 7.999.999.999.999.999 there, thus it's not been 'clean' in the past ... 

no 'fp-imprecision' neccessary in that range, and ... rounding should in some way help to cover imprecision, not introduce additional fails ...
Comment 2 zcrhonek 2020-11-20 14:25:33 UTC
This seems to have begun at the below commit.
Adding Cc: to Eike Rathke ; Could you possibly take a look at this one?
 4ac0c8c10e88ac1ea470606e0d6973f8171c8516 is the first bad commit
commit 4ac0c8c10e88ac1ea470606e0d6973f8171c8516
Author: Jenkins Build User <tdf@pollux.tdf>
Date:   Thu Mar 28 23:23:07 2019 +0100

    source sha:edcbe8c4e02a67c74ec6f85f28899431dbfa0765

tdf#124286 fix annoying rounding error.

In case of ROUNDDOWN and ROUNDUP, it is possible that seemingly clear decimal
values are rounded unexpectedly (from the user's POV). This is caused by the i
decimal to binary to decimal conversions.
By rounding to 12 significanr digits before calling the round-down of -up
function, most of these unexpected roundings are eliminated.

Change-Id: Ia19181383b77e1ff40a067c4a1cea1ece0955871
Reviewed-on: https://gerrit.libreoffice.org/69762
Tested-by: Jenkins
Reviewed-by: Eike Rathke <erack@redhat.com>
Comment 3 Eike Rathke 2020-11-30 23:12:49 UTC
Input of
results in -999.13 and the formula expression becomes
If you force a 0.000000000000 number format the resulting -999.130000000010 is a display string conversion problem, not a wrong calculation.

Input of
results in -999.13 and the formula expression becomes

The input of -999.12999999999994 can't be represented in IEEE-754 double floating-point and yields -999.13

The nearest representable value of -999.12999999999994 is -999.1299999999999954525264911353588104248046875 with a raw hex value of c08f390a3d70a3d7 (binary exponent.mantissa -1111100111.0010000101000111101011100001010001111010111) which is the same nearest value as for -999.13
Do not expect rounding down to whatever decimals would yield any -999.12999999999990

This is no way "critical, data corruption", it is how IEEE-754 double floating-point works.

I also get no different results in 6.1.6 apart from that the forced 12 decimals formatted string for the first case is -999.130000000000

@zcrhonek: what and how did you test there to get to the bisected result?
Comment 4 zcrhonek 2020-12-01 16:34:01 UTC
Created attachment 167728 [details]
test file

Eike, test file.
Comment 5 b. 2020-12-06 19:25:02 UTC
the display strings should / could be better? 

'=ROUNDUP(999,1300000004;2)'   -> 999,13000000000 is not ok?, 

'=ROUNDDOWN(999,1299999995;2)' -> 999,13000000000 is not ok?, 

above results from, both better with, 

sorry, OP was just the first occurence without more research,
Comment 6 b. 2021-02-04 14:03:15 UTC
@erAck: yes, but '=ROUNDUP(-999,13;11)' shouldn't end in -999,13000000001000000000 either... right? but it does :-(  
and it's not just 'display string', see:  

=RAWSUBTRACT(ROUNDUP(-999,13;11);-999,13) -> -0,00000000001000444172  


'=RAWSUBTRACT(-999,13;-999,13)' -> 0,0000000000000000
'=RAWSUBTRACT(ROUND(-999,13;2);-999,13)' -> 0,00000000000000000000

there are two distinct issues:  

1. the patch by @Winfried Donkers - https://gerrit.libreoffice.org/c/core/+/69762 - disables accuracy for values with more than 12 significant digits, such is a very questionable strategy, i would suggest to undo the patch and fix the errors in the elementary functions instead (roundsig and subtraction)), 
2. something injects a small deviation into these calculations, imho it starts with roundsig being inaccurate, see: 

'=RAWSUBTRACT(ROUNDSIG(-999,13;12);-999,13)' -> -0,00000000000011368684  

('RoundSignificant( fX, 12, fRes );' is used in the patch)
Comment 7 b. 2021-05-22 10:32:10 UTC
it sounds like magic, but with that little change: 

                // tdf124286 : round to 12 significant digits before rounding
                //             down or up to avoid unexpected rounding errors
                //             caused by decimal -> binary -> decimal conversion
                // [edit b. 2021-05-22: try with roundsig 15 instead of roundsig 12 to keep precision]
                double fRes;
                //ori was: RoundSignificant( fX, 12, fRes );
                RoundSignificant( fX, 15, fRes );
                fVal = ::rtl::math::round( fRes, nDec, eMode );

instead of: 

                // tdf124286 : round to 12 significant digits before rounding
                //             down or up to avoid unexpected rounding errors
                //             caused by decimal -> binary -> decimal conversion
                double fRes;
                RoundSignificant( fX, 12, fRes );
                fVal = ::rtl::math::round( fRes, nDec, eMode );

in sc/source/core/tool/interpr2.cxx 

the above issues seem gone, while the 'healing' against the cancellation issue from tdf#124286 (8,94-8) still holds ... 

would one of the more experienced users or devs like to: 
- recheck, 
- if good lead me through the process of implementing it as a patch?  

(tried with Kali (Debian) Linux and master source from 2021-05-20)
Comment 8 Eike Rathke 2021-05-23 13:01:29 UTC
It breaks the examples you gave in https://bugs.documentfoundation.org/show_bug.cgi?id=124286#c17

=ROUNDDOWN(2-5E-015;14) -> 1.99999999999999
=ROUNDDOWN(2-5E-015;13) -> 1.9999999999999

Both are then 2, also if nDec < 12 is changed to nDec < 15.

However, process to setup Gerrit and submit patches is described at https://wiki.documentfoundation.org/Development/gerrit