Download it now!
Bug 101837 - EDITING: Calculation error for CHIINV function
Summary: EDITING: Calculation error for CHIINV function
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
5.3.0.0.alpha0+
Hardware: x86 (IA32) All
: medium normal
Assignee: Not Assigned
URL: http://nabble.documentfoundation.org/...
Whiteboard:
Keywords:
Depends on:
Blocks: Calc-Function Calculate Dev-Bugs
  Show dependency treegraph
 
Reported: 2016-09-01 07:44 UTC by Alex Kempshall
Modified: 2019-08-12 21:55 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:


Attachments
updated unittest document for CHIINV (23.24 KB, application/vnd.oasis.opendocument.spreadsheet)
2018-04-15 07:04 UTC, Winfried Donkers (retired)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Kempshall 2016-09-01 07:44:40 UTC
When compiling LibreOffice I get a failed Unit test. The problem occurs in  sc/qa/unit/functions_test.cxx with

CPPUNIT_ASSERT_DOUBLES_EQUAL(1.0, rDoc.GetValue(1, 2, 0), 1e-14); 

whilst testing sc/qa/unit/data/functions/fods/chiinv.fods. 

The problem appears to be related to row 8 of sheet 2 in chiinv.fods.

In 5.1.4.2 Libreoffice returns the expected result 0.005035400390625

In 5.3.0.0.alpha0+ Libreoffice returns a non-expected result 0.00509040527300573


I know nothing about the CHIINV function and even when I tried a online CHIINV calculator was still none the wiser and also got different results to either of the above.
Comment 1 Eike Rathke 2016-09-02 12:09:57 UTC
So this seems to point out the weird build failures we had with the function test documents that weren't reproducible on other systems. Adding interests to Cc ...
Comment 2 Alex Kempshall 2016-09-02 15:29:30 UTC
Had another look at this one and it suspiciously looks looks a rounding error. Pushed the numbers through the medcalc website and compared it with Calc. The precision on medcalc is 9 digits after the decimal point whereas for calc it's 15 digits after the decimal point. I've increased the precision each time from 0.9 to 0.999999999999999. Everything seems in order until we get to the last iteration. 

Calc                    MedCalc        

4.86518205192533	4.865182052	FALSE	=CHIINV(F25,G25)	1	0.9	10
2.55821216018721	2.55821216	FALSE	=CHIINV(F26,G26)	2	0.99	10
1.47874346383567	1.478743464	FALSE	=CHIINV(F27,G27)	3	0.999	10
0.888920357912875	0.888920358	FALSE	=CHIINV(F28,G28)	4	0.9999	10
0.545169540878432	0.545169541	FALSE	=CHIINV(F29,G29)	5	0.99999	10
0.338126003244954	0.338126003	FALSE	=CHIINV(F30,G30)	6	0.999999	10
0.211104115873973	0.211104116	FALSE	=CHIINV(F31,G31)	7	0.9999999	10
0.132327944421752	0.132327944	FALSE	=CHIINV(F32,G32)	8	0.99999999	10
0.083152274011688	0.083152274	FALSE	=CHIINV(F33,G33)	9	0.999999999	10
0.05233106650163	0.052331067	FALSE	=CHIINV(F34,G34)	10	0.9999999999	10
0.032965528624475	0.032965455	FALSE	=CHIINV(F35,G35)	11	0.99999999999	10
0.020778597606181	0.020778598	FALSE	=CHIINV(F36,G36)	12	0.999999999999	10
0.013099986480001	0.0131029	FALSE	=CHIINV(F37,G37)	13	0.9999999999999	10
0.008262202343253	0.008262202	FALSE	=CHIINV(F38,G38)	14	0.99999999999999	10
0.005090405273006	0.005211772	FALSE	=CHIINV(F39,G39)	15	0.999999999999999	10
Comment 3 Eike Rathke 2016-09-08 13:01:17 UTC
See also recent tinderbox Linux-rpm_deb-x86_71-TDF and Linux-rpm_deb-x86_71-TDF-dbg builds.
Comment 4 Luke 2016-09-12 05:00:22 UTC
Alex are you sure 5.1.4.2 is passing? Are you pressing Shift+Ctrl+F9? For me the 32-bit Version: 5.1.4.2 Build ID: 1:5.1.4-0ubuntu1 and Excel both fail.

5.1.4.2 Ubuntu Calc          A8=0.0050904053
Excel 2013                   A8=0.0050903836
Gnumeric                     A8=0.0050903836
Expected/Win32/Win64/Linx64  A8=0.0050354004

It seems that our 32-bit Linux floating point math is closer to Excel than our 64-bit. Should 32 and 64-bit builds get the same results? Or should we drop the accuracy of the test? 
C8=ROUND(A8,3)=ROUND(B8,3) 
Will pass all versions. 


CHIINV Was added with:
commit 1fe44f6e9763203689c6d98aef82b5141d95410e
Author: Zdeněk Crhonek <zcrhonek@gmail.com>
Date:   Thu Aug 18 22:56:35 2016 +0200

Is this when the 32-bit tinderboxes started failing?
Comment 5 Alex Kempshall 2016-09-12 08:52:41 UTC
Luke


LO Version 5.3.0.0.alpha 0+ compiled from source on 32bit Lubuntu 14.04LTS, the chiinv calculation results in 0.005090405273006

LO Version 5.3.0.0.alpha 0+ compiled from source on 32bit Slackware 14.2 ( a few months out of date), the chiinv calculation results in 0.005090405273006

LO Version 5.0.2.2 downloaded from Fresh some time ago on 32bit Slackware 14.2 ( a few months out of date), the chiinv calculation results in 0.0050904053

LO Version 5.0.0.5 installed using apt-get on 32bit Lubuntu 14.04LTS, the chiinv calculation results in 0.0050904053

LO Version 5.1.4.2 downloaded from Still on an up to date 32bit Slackware 14.1, the chiinv calculation  results in 0.0050354004

LO Version 5.1.4.2 downloaded from Still on 32bit Slackware 14.2 ( a few months out of date), the chiinv calculation results in 0.0050904053

The number of places after the decimal point is something I may have changed under options or some such place. Can't find where that option was now! Anyway I seem to remember that I reduced it from something like 15 to 12, don't rely on my memory it could have been 15 to 10. As I say I can't remember where I changed it or on what version. Probably not important as the results diverge after the 5th decimal place.


So at the moment it looks like that LO version 5.1.4.2 calculates -

0.0050904053 on 32bit Slackware 14.2 ( a few months out of date)
0.0050354004 on an up to date 32bit Slackware 14.1

In other words it looks to me that the same binaries give different results on different platforms.

Everything above other than the up to date 32bit Slackware 14.1 (my production platform) is running in VirtualBox

Alex
Comment 6 Eike Rathke 2016-09-20 13:41:38 UTC
(In reply to Luke from comment #4)
> Alex are you sure 5.1.4.2 is passing? Are you pressing Shift+Ctrl+F9? For me
> the 32-bit Version: 5.1.4.2 Build ID: 1:5.1.4-0ubuntu1 and Excel both fail.

Note that the function tests in question are not part of 5.1, this is specifically about current master 32-bit builds failing.

Anyway, we now disabled the tests for 32-bit builds until a more detailed solution is available as not only one or two tests are affected.
Comment 7 Alex Kempshall 2016-09-22 16:07:28 UTC
On 16/09/16 11:05, Stephan Bergmann wrote:
>
> Turns out, the relevant GCC switches are
>
>   -mfpmath=sse -msse2
>

On this basis I ran

make clean && make ENVCFLAGS="-mfpmath=sse -msse2" ENVCFLAGSCXX="-mfpmath=sse -msse2" && make sc.check

The compile and tests all completed successfully on an up to date 32-bit Lubuntu 14.04 and 32-bit Slackware-Current from November 2015. I haven't pulled any new code since 1st September so haven't inadvertently picked up the work around about disabling the tests.
Comment 8 Aron Budea 2018-04-14 13:39:50 UTC
I was going to open a new bug report, when stumbled upon this. It's reproducible with a 32-bit master (0fe5d61ad2c9e21f393bdad4b706e398728a70d8) Windows debug build for me.

- Open test files from 'sc\qa\unit\data\functions\statistical\fods', the ones from 'avedev.fods' to 'chiinv.fods' (16 files altogether, in alphabetical order).
- Do a full recalc of 'chiinv.fods'.

=> Sheet2's C8 becomes false. CHIINV(0.999999999999999; 10) is evaluated to 0.005090405273006 instead of the expected 0.005035400390625.

After further opening/closing files, sometimes I get errors on failed thread creation ("osl::Thread::create failed", in the logs: "_beginthreadex undocumented errno ENOMEM - this means not enough VM for stack"). This might be a red herring, I'd assume the unit tests are executed one by one, and don't run out of resources.
Comment 9 Winfried Donkers (retired) 2018-04-15 06:52:31 UTC
In 2017 a function ROUNDSIG has been introduced to cope with rounding problems like these. I will update the unittest document for CHIINV, but cannot test it on 32 bit systems.

@Alex, @Aaron: if I upload  an updated unittest document as attachment to this bug report, can you test if this updated document no longer has  the reported problems?
Comment 10 Winfried Donkers (retired) 2018-04-15 07:04:44 UTC
Created attachment 141374 [details]
updated unittest document for CHIINV

Updated unittest document for CHIINV to be tested on 32bit platforms.
Open document and recalculate entire document, all result in column B of tab 1 should be green (TRUE). If not, please inform me which rows in tab 2 are red (FALSE) in column C.
Comment 11 Aron Budea 2018-04-15 15:31:10 UTC
(In reply to Winfried Donkers from comment #10)
> Updated unittest document for CHIINV to be tested on 32bit platforms.
> Open document and recalculate entire document, all result in column B of tab
> 1 should be green (TRUE). If not, please inform me which rows in tab 2 are
> red (FALSE) in column C.

Thanks for taking a stab at this! Unfortunately the issue is the same with the ROUNDSIG equality check, C8 is false when the other 15 unit test spreadsheets are also open together with the new one.
Comment 12 Aron Budea 2018-04-17 05:37:17 UTC
Winfried, Eike, could either of you please point me to where CHIINV is calculated in the code? I'd love to debug into this to see what's going on.
Comment 13 Eike Rathke 2018-04-17 10:47:12 UTC
It's in sc/source/core/tool/interpr3.cxx ScInterpreter::ScChiInv() and the calls down from lcl_IterateInverse() using ScChiDistFunction and ScInterpreter::GetChiDist(). Note that the lcl_IterateInverse() code is shared among several distributions, also the Gamma functions.
Comment 14 Winfried Donkers (retired) 2018-04-17 11:15:22 UTC
Unassigning myself, as a quick fix proved not to fix anything at all and I find it hard to find time to work on it in the very near future.

@Aaron: Should you have question, please feel free to ask. No guarantee that I can answer within hours, nor that I will know the answer ;-)
Comment 15 Aron Budea 2018-04-18 05:50:28 UTC
Very peculiar, in both cases the first 13 iterations of the while loop in lcl_IterateInverse(...) ("inverse quadric interpolation with additional brackets") are the exact same.

Then in the 14th iteration, this diverges:
fRy = rFunction.GetValue(fSx);    <= rfunction is ScChiDistFunction

fRy becomes:
- in the good case: 0
- in the bad case: -4.6945954068622342e-17

However, in both cases the result of ScChiDistFunction::GetValue(...) is supposed to be:
0.99999999999999911 - 0.99999999999999911
(return fp - rInt.GetChiDist(x, fDF);)

Surely the displayed value doesn't reflect the actual double value.


In the good case, the while loop exits afterwards, because this part of the condition is false: fabs(fRy) > fYEps
In the bad case the while loop does an extra 3 iterations, which results in the final discrepancy.

For the record, in the bad case the equality check only passes with 1 digit of rounding, ie. ROUNDSIG(A8;1)
Comment 16 Xisco Faulí 2018-04-18 15:39:46 UTC
Assigning it to Aron for the time being...
Comment 17 Aron Budea 2018-04-18 19:20:51 UTC
This limit in lcl_IterateInverse seems to be extremely tight:
fYEps = 1.0E-307;

Could this be set to ::std::numeric_limits<double>::epsilon() as well?

The value was originally added in this commit:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=07e6b192bc9e13ecf29bffda40ef371fc4a944a0
Comment 18 Eike Rathke 2018-04-19 09:16:26 UTC
(In reply to Aron Budea from comment #17)
> This limit in lcl_IterateInverse seems to be extremely tight:
> fYEps = 1.0E-307;
> 
> Could this be set to ::std::numeric_limits<double>::epsilon() as well?
> 
> The value was originally added in this commit:
> https://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=07e6b192bc9e13ecf29bffda40ef371fc4a944a0

Adding Regina to Cc who is the original author there.
I think double epsilon() isn't a suitable replacement at all, given that it is "the difference between 1.0 and the next value representable", which for IEEE double is 2^-52 ~= 2.22045E-16
Comment 19 Aron Budea 2018-04-19 21:19:05 UTC
Let me unassign myself to avoid any confusion.
Comment 20 sudhanshu arora 2019-04-28 06:15:54 UTC Comment hidden (spam)