[7.6][ML] Correct logistic loss function (#1059)

Backport #1032.

Author: tveasey

docs/CHANGELOG.asciidoc

@@ -28,6 +28,13 @@
 
 //=== Regressions
 
+== {es} version 7.6.2
+
+=== Bug Fixes
+
+* Fix a bug in the calculation of the minimum loss leaf values for classification.
+(See {ml-pull}1032[#1032].)
+
 == {es} version 7.6.0
 
 === New Features

include/maths/CBoostedTree.h

@@ -97,8 +97,10 @@ class MATHS_EXPORT CArgMinLogisticImpl final : public CArgMinLossImpl {
     }
 
     double bucketWidth() const {
-        return m_PredictionMinMax.range() /
-               static_cast<double>(m_BucketCategoryCounts.size());
+        return m_PredictionMinMax.initialized()
+                   ? m_PredictionMinMax.range() /
+                         static_cast<double>(m_BucketCategoryCounts.size())
+                   : 0.0;
     }
 
 private:

lib/maths/CBoostedTree.cc

@@ -154,11 +154,17 @@ double CArgMinLogisticImpl::value() const {
     // case we only need one pass over the data and can compute the optimal
     // value from the counts of the two categories.
     if (this->bucketWidth() == 0.0) {
-        objective = [this](double weight) {
+        // This is the (unique) predicted value for the rows in leaf by the forest
+        // so far (i.e. without the weight for the leaf we're about to add).
+        double prediction{m_PredictionMinMax.initialized()
+                              ? (m_PredictionMinMax.min() + m_PredictionMinMax.max()) / 2.0
+                              : 0.0};
+        objective = [prediction, this](double weight) {
+            double logOdds{prediction + weight};
             double c0{m_CategoryCounts(0)};
             double c1{m_CategoryCounts(1)};
             return this->lambda() * CTools::pow2(weight) -
-                   c0 * logOneMinusLogistic(weight) - c1 * logLogistic(weight);
+                   c0 * logOneMinusLogistic(logOdds) - c1 * logLogistic(logOdds);
         };
 
         // Weight shrinkage means the optimal weight will be somewhere between
@@ -168,8 +174,8 @@ double CArgMinLogisticImpl::value() const {
         double empiricalProbabilityC1{c1 / (c0 + c1)};
         double empiricalLogOddsC1{
             std::log(empiricalProbabilityC1 / (1.0 - empiricalProbabilityC1))};
-        minWeight = empiricalProbabilityC1 < 0.5 ? empiricalLogOddsC1 : 0.0;
-        maxWeight = empiricalProbabilityC1 < 0.5 ? 0.0 : empiricalLogOddsC1;
+        minWeight = (empiricalProbabilityC1 < 0.5 ? empiricalLogOddsC1 : 0.0) - prediction;
+        maxWeight = (empiricalProbabilityC1 < 0.5 ? 0.0 : empiricalLogOddsC1) - prediction;
 
     } else {
         objective = [this](double weight) {

lib/maths/CBoostedTreeImpl.cc

@@ -1241,7 +1241,9 @@ void CBoostedTreeImpl::refreshPredictionsAndLossDerivatives(core::CDataFrame& fr
     } while (nextPass());
 
     for (std::size_t i = 0; i < tree.size(); ++i) {
-        tree[i].value(eta * leafValues[i].value());
+        if (tree[i].isLeaf()) {
+            tree[i].value(eta * leafValues[i].value());
+        }
     }
 
     LOG_TRACE(<< "tree =\n" << root(tree).print(tree));

lib/maths/unittest/CBoostedTreeTest.cc

@@ -1123,7 +1123,7 @@ BOOST_AUTO_TEST_CASE(testLogisticRegression) {
         LOG_DEBUG(<< "log relative error = "
                   << maths::CBasicStatistics::mean(logRelativeError));
 
-        BOOST_TEST_REQUIRE(maths::CBasicStatistics::mean(logRelativeError) < 0.75);
+        BOOST_TEST_REQUIRE(maths::CBasicStatistics::mean(logRelativeError) < 0.71);
         meanLogRelativeError.add(maths::CBasicStatistics::mean(logRelativeError));
     }
 
@@ -1215,7 +1215,7 @@ BOOST_AUTO_TEST_CASE(testImbalancedClasses) {
     LOG_DEBUG(<< "recalls    = " << core::CContainerPrinter::print(recalls));
 
     BOOST_TEST_REQUIRE(std::fabs(precisions[0] - precisions[1]) < 0.1);
-    BOOST_TEST_REQUIRE(std::fabs(recalls[0] - recalls[1]) < 0.15);
+    BOOST_TEST_REQUIRE(std::fabs(recalls[0] - recalls[1]) < 0.13);
 }
 
 BOOST_AUTO_TEST_CASE(testEstimateMemoryUsedByTrain) {