Bug 99730 - testFuncMDETERM fails due to floating point calculations
Summary: testFuncMDETERM fails due to floating point calculations
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
5.1.2.2 release
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Eike Rathke
URL:
Whiteboard: target:5.2.0 target:5.1.4
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-05-08 13:50 UTC by librenedine
Modified: 2016-10-25 19:03 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
Possible patch using a fuzzy test for zero (1.28 KB, patch)
2016-05-08 13:50 UTC, librenedine
Details

Note You need to log in before you can comment on or make changes to this bug.
Description librenedine 2016-05-08 13:50:29 UTC
Created attachment 124910 [details]
Possible patch using a fuzzy test for zero

I get the following error when running the test suite on libreoffice-5.1.2.2

/sc/qa/unit/ucalc_formula.cxx:6841:Test::testFuncMDETERM
equality assertion failed
- Expected: 0
- Actual  : 6.34413156928661e-17
- Calculation of MDETERM incorrect for singular integer matrix

ucalc_formula.cxx:6841:Assertion
Test name: Test::testFuncMDETERM
equality assertion failed
- Expected: 0
- Actual  : 6.34413156928661e-17
- Calculation of MDETERM incorrect for singular integer matrix

Failures !!!
Run: 224   Failure total: 1   Failures: 1   Errors: 0


The system is x86_64 Linux with a Intel(R) Core(TM) i5-5200U CPU @ 2.20GHz CPU
using GCC 4.8.5. The error is reproducable on my system.

Commits introducing this test are
c43442dfef3f5b21bec40054b56f406efdc5a84f
8140309d636d4a870875f2dd75ed3dfff2c0fbaf  (*)
222b2f9370bfc05473249ab3f78e9adca67754bf  (**)

Related bug is #32834.

The first commit introduces the test using an equality test for zero, (*) fixes
this issue by using a fuzzy test for zero, while (**) removes this test for
64bit platforms.

Note that is is a very bad idea to check for equality on floating point values
and it should always be a fuzzy test (such as in (*)). This even applies if one
can guarantee that the double was converted from an integer value (as in the
test case).

I propose to revert 222b2f9370bfc05473249ab3f78e9adca67754bf (reintroducing a
error bound of 1e-4) or applying the patch below which introduces an error
tolerance of 1e-12. (And also states the error tolerance in the error message.)

Note:
Using the LU (or LUP) decomposition to calculate the determinant of a matrix is
sometimes numerically unstable and can be wrong by an order of magnitude.
Cf section 'Limitations' of https://www.mathworks.com/help/matlab/ref/det.html.

Patch (also attached):

diff -u a/sc/qa/unit/ucalc_formula.cxx b/sc/qa/unit/ucalc_formula.cxx
--- a/sc/qa/unit/ucalc_formula.cxx
+++ b/sc/qa/unit/ucalc_formula.cxx
@@ -6847,17 +6847,10 @@
         aFormulaBuffer[13] = static_cast<sal_Unicode>( '0' + nSize );
         m_pDoc->SetString(aPos, aFormulaBuffer.toString());

-        // On crappy 32-bit targets, presumably without extended precision on
-        // interim results or optimization not catching it, this test fails
-        // when comparing to 0.0, so have a narrow error margin. See also
-        // commit message of 8140309d636d4a870875f2dd75ed3dfff2c0fbaf
-#if SAL_TYPES_SIZEOFPOINTER == 4
-        CPPUNIT_ASSERT_DOUBLES_EQUAL_MESSAGE("Calculation of MDETERM incorrect for singular integer matrix",
+        // Do not compare with equality instead use an error margin.
+        // This applies to both 32bit and 64bit.
+        CPPUNIT_ASSERT_DOUBLES_EQUAL_MESSAGE("Calculation of MDETERM incorrect for singular integer matrix (error tolerance 1e-12)",
                 0.0, m_pDoc->GetValue(aPos), 1e-12);
-#else
-        CPPUNIT_ASSERT_EQUAL_MESSAGE("Calculation of MDETERM incorrect for singular integer matrix",
-                0.0, m_pDoc->GetValue(aPos));
-#endif
     }

     int aVals[] = {23, 31, 13, 12, 34, 64, 34, 31, 98, 32, 33, 63, 45, 54, 65, 76};
Comment 1 Aron Budea 2016-05-12 16:06:15 UTC
Miklós, I remember you having a discussion on IRC about this piece of code earlier today (not about this exact change). What's your take on the suggestion?
Comment 2 Markus Mohrhard 2016-05-13 01:51:25 UTC
I very much doubt that Miklos is interested in calc issues.

Normally I would agree with this patch but it seems that Eike changed the code back after sberg implemented a similar patch.

@Eike: Is there any reason why we should not use a 1e-15 or 1e-14 tolerance for the check.
Comment 3 Eike Rathke 2016-05-13 11:07:59 UTC
The reason I did it that way probably was that the determinant for the specific matrix set SHOULD be 0.0 even with our LU decomposition, and so far WAS, except for the 32 bit case and now librenedine's case. Which is odd. Btw, what system still uses gcc 4.8.5? Even Debian Jessie has 4.9.2 ...

So, I'm not against a general error margin of 10e-12, which we also use at other places, if this helps to build. I'm just sad we have to lower the bar to cater for inferior systems ;-)
Comment 4 Eike Rathke 2016-05-13 11:18:30 UTC
Oh and I changed the original tolerance of 8140309d636d4a870875f2dd75ed3dfff2c0fbaf because 10e-4 wouldn't detect anything wrong in this case..

I think I'll go with 10e-14 here.
Comment 5 Eike Rathke 2016-05-13 11:36:22 UTC
Seeing bug 32834 even lowering to 10e-14 would be nonsense because the original complaint was about -9.51712667007776E-016

@librenedine: so what makes your system so special? Anything we could detect without effectively disabling the test?
Comment 6 David Tardon 2016-05-13 12:28:54 UTC
This causes build failures of Fedora 24/Rawhide on aarch64, ppc64* and s390x. Using 1e-12 delta makes the pass at least on ppc64le.
Comment 7 Commit Notification 2016-05-13 13:19:39 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=b35b601d9e3b43eaedb8576b70d10b657f625d6e

Resolves: tdf#99730 lower the barrier for inferior systems, cripple the test

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 8 Eike Rathke 2016-05-13 13:33:07 UTC
Pending review https://gerrit.libreoffice.org/24971 for 5-1
Comment 9 Commit Notification 2016-05-13 19:18:20 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=1d2091b91e91b50eb98f41c7659746a47cb70c73&h=libreoffice-5-1

Resolves: tdf#99730 lower the barrier for inferior systems, cripple the test

It will be available in 5.1.4.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 10 librenedine 2016-05-17 19:18:09 UTC
Eike Rathke wrote:
> @librenedine: so what makes your system so special? Anything we could detect
> without effectively disabling the test?

Honestly I don't know what's so special about my system.
If it helps, I can run the compilation again with a different compiler version and with different Linux OSs.

This might take some time, though. But I hope I can post some results at the beginning of next week.