Fix bugs in the OpenCL implementation of some statistical functions

Don't return negative values from ScGroupTokenConverter::trimLength().  It
doesn't make sense, we should return zero instead. At the two call sites,
there are tests against a zero having been returned, but not against a
negative value. And the return value is even passed as the nArrayLength
parameter to a formula::DoubleVectorRefToken constructor, which is of type
size_t, thus unsigned. Passing for instance -4 to it ends up being interpreted
as 18446744073709551612, which has fun consequences. I got a crash from a
spreadsheet with formulas that referenced some empty cells.

Set #VALUE! error in COVAR() and PEARSON() OpenCL implementation when
appropriate. Returning a "bare" NaN with no "double error" semantic payload
does not make sense. Bare NaNs are displayed in Calc as 'nan' which probably
is not what we want.

Set #VALUE! error in COVAR() OpenCL implementation when appropriate. Returning
-DBL_MAX doesn't make sense. The traditional C++ implementation and other
spreadsheet products return an error.

Set #DIV/0! error in VAR() OpenCL implementation when appropriate. Returning
DBL_MAX doesn't make sense. The traditional C++ implementation and other
spreadsheet products return an error.

Return a #DIV/0! error in one case and #VALUE! in another in the OpenCL
SLOPE() implementation instead of bare NANs. There are still many places in
this function where the code bluntly returns a bare NAN. That is always the
wrong thing to do. However, it is not certain which error code is the right
error in each case. One would have to check in each case how to get to that
place in the code, and compare to what the reference C++ implementation and
other spreadsheet products do in each case.

Return #VALUE! instead of NaN in the OpenCL NORMSINV()

Add NORMSDIST, VAR, CORREL, COVAR, PEARSON and SLOPE to the OpenCL-enabled
default opcode subset. Having these statistical functions perform fast is
essential in many cases, and their implementations seem to be correct now.

Change-Id: Idb202756f5b64e30b9bb87c00e340b8060694509
Reviewed-on: https://gerrit.libreoffice.org/14769
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Tested-by: Miklos Vajna <vmiklos@collabora.co.uk>

3 files changed, 27 insertions, 6 deletions

diff --git a/sc/source/core/data/grouptokenconverter.cxx b/sc/source/core/data/grouptokenconverter.cxx
index bfb6de359c54..da3964cf9864 100644
--- a/sc/source/core/data/grouptokenconverter.cxx
+++ b/sc/source/core/data/grouptokenconverter.cxx
@@ -63,7 +63,19 @@ SCROW ScGroupTokenConverter::trimLength(SCTAB nTab, SCCOL nCol1, SCCOL nCol2, SC
     SCROW nLastRow = nRow + nRowLen - 1; // current last row.
     nLastRow = mrDoc.GetLastDataRow(nTab, nCol1, nCol2, nLastRow);
     if (nLastRow < (nRow + nRowLen - 1))
+    {
+        // This can end up negative! Was that the original intent, or
+        // is it accidental? Was it not like that originally but the
+        // surrounding conditions changed?
         nRowLen = nLastRow - nRow + 1;
+        // Anyway, let's assume it doesn't make sense to return a
+        // negative value here. But should we then return 0 or 1? In
+        // the "Column is empty" case below, we return 1, why!? And,
+        // at the callsites there are tests for a zero value returned
+        // from this function (but not for a negative one).
+        if (nRowLen < 0)
+            nRowLen = 0;
+    }
     else if (nLastRow == 0)
         // Column is empty.
         nRowLen = 1;
diff --git a/sc/source/core/opencl/op_statistical.cxx b/sc/source/core/opencl/op_statistical.cxx
index 7fe8eedd56bb..0b7c572261dc 100644
--- a/sc/source/core/opencl/op_statistical.cxx
+++ b/sc/source/core/opencl/op_statistical.cxx
@@ -248,7 +248,7 @@ void OpVar::GenSlidingWindowFunction(std::stringstream &ss,
         }
     }
     ss << "    if (fCount <= 1.0)\n";
-    ss << "        return DBL_MAX;\n";
+    ss << "        return CreateDoubleError(errDivisionByZero);\n";
     ss << "    else\n";
     ss << "        return vSum * pow(fCount - 1.0,-1.0);\n";
     ss << "}\n";
@@ -3283,7 +3283,7 @@ void OpSlope::GenSlidingWindowFunction(std::stringstream &ss,
         ss << "    }\n";
 
         ss << "    if (fCount < 1.0)\n";
-        ss << "        return NAN;\n";
+        ss << "        return CreateDoubleError(errNoValue);\n";
         ss << "    else\n";
         ss << "    {\n";
         ss << "        fMeanX = fSumX * pow(fCount,-1.0);\n";
@@ -3349,7 +3349,7 @@ void OpSlope::GenSlidingWindowFunction(std::stringstream &ss,
         ss << "            fSumSqrDeltaX += (argX-fMeanX) * (argX-fMeanX);\n";
         ss << "        }\n";
         ss << "        if(fSumSqrDeltaX == 0.0)\n";
-        ss << "            return NAN;\n";
+        ss << "            return CreateDoubleError(errDivisionByZero);\n";
         ss << "        else\n";
         ss << "        {\n";
         ss << "            return fSumDeltaXDeltaY*pow(fSumSqrDeltaX,-1.0);\n";
@@ -4048,6 +4048,8 @@ void OpPearson::GenSlidingWindowFunction(
     ss << "       }\n";
     ss << "      double tmp = ( fSumDeltaXDeltaY / ";
     ss << "sqrt( fSumX * fSumY));\n\t";
+    ss << "      if (isnan(tmp))\n";
+    ss << "          return CreateDoubleError(errNoValue);\n";
     ss << "      return tmp;\n";
     ss << "}\n";
 }
@@ -5594,8 +5596,9 @@ void OpNormsinv:: GenSlidingWindowFunction
     ss <<"}\n";
     ss << "z = q < 0.0 ? (-1)*z : z;\n";
     ss <<"}\n";
-    ss <<"double tmp = z;\n";
-    ss <<"return tmp;\n";
+    ss <<"if (isnan(z))\n";
+    ss <<"    return CreateDoubleError(errNoValue);\n";
+    ss <<"return z;\n";
     ss <<"}\n";
 }
 void OpMedian::GenSlidingWindowFunction(
@@ -8191,7 +8194,7 @@ void OpCovar::GenSlidingWindowFunction(std::stringstream& ss,
                 ss << "    }\n";
             }
             ss << "    if(cnt < 1) {\n";
-            ss << "        return -DBL_MAX;\n";
+            ss << "        return CreateDoubleError(errNoValue);\n";
             ss << "    }\n";
             ss << "    else {\n";
             ss << "        vMean0 = vSum0 / cnt;\n";
diff --git a/sc/source/core/tool/calcconfig.cxx b/sc/source/core/tool/calcconfig.cxx
index 144d5a8423bd..f1e7bf034788 100644
--- a/sc/source/core/tool/calcconfig.cxx
+++ b/sc/source/core/tool/calcconfig.cxx
@@ -53,6 +53,7 @@ void ScCalcConfig::setOpenCLConfigToDefault()
     maOpenCLSubsetOpCodes.insert(ocExp);
     maOpenCLSubsetOpCodes.insert(ocLn);
     maOpenCLSubsetOpCodes.insert(ocSqrt);
+    maOpenCLSubsetOpCodes.insert(ocStdNormDist);
     maOpenCLSubsetOpCodes.insert(ocSNormInv);
     maOpenCLSubsetOpCodes.insert(ocRound);
     maOpenCLSubsetOpCodes.insert(ocPower);
@@ -63,7 +64,12 @@ void ScCalcConfig::setOpenCLConfigToDefault()
     maOpenCLSubsetOpCodes.insert(ocProduct);
     maOpenCLSubsetOpCodes.insert(ocAverage);
     maOpenCLSubsetOpCodes.insert(ocCount);
+    maOpenCLSubsetOpCodes.insert(ocVar);
     maOpenCLSubsetOpCodes.insert(ocNormDist);
+    maOpenCLSubsetOpCodes.insert(ocCorrel);
+    maOpenCLSubsetOpCodes.insert(ocCovar);
+    maOpenCLSubsetOpCodes.insert(ocPearson);
+    maOpenCLSubsetOpCodes.insert(ocSlope);
     maOpenCLSubsetOpCodes.insert(ocSumIfs);
 }