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};
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?
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.
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 ;-)
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.
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?
This causes build failures of Fedora 24/Rawhide on aarch64, ppc64* and s390x. Using 1e-12 delta makes the pass at least on ppc64le.
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.
Pending review https://gerrit.libreoffice.org/24971 for 5-1
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.
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.