fix: correct LightGBM improvement tolerance semantics

lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/TrainUtils.scala

@@ -11,7 +11,26 @@ import org.slf4j.Logger
 import java.io._
 import scala.annotation.tailrec
 
-private object TrainUtils extends Serializable {
+private[lightgbm] object TrainUtils extends Serializable {
+
+  private val HigherIsBetterMetricPrefixes = Seq("auc", "ndcg@", "map@", "average_precision")
+
+  private[lightgbm] def isImprovement(evalName: String,
+                                      evalScore: Double,
+                                      bestScore: Double,
+                                      improvementTolerance: Double): Boolean = {
+    if (HigherIsBetterMetricPrefixes.exists(evalName.startsWith)) {
+      evalScore - bestScore > improvementTolerance
+    } else {
+      bestScore - evalScore > improvementTolerance
+    }
+  }
+
+  private[lightgbm] def shouldStopEarly(iteration: Int,
+                                        bestIteration: Int,
+                                        earlyStoppingRound: Int): Boolean = {
+    earlyStoppingRound > 0 && iteration - bestIteration >= earlyStoppingRound
+  }
 
   def createBooster(trainParams: BaseTrainParams,
                     trainDataset: LightGBMDataset,
@@ -144,20 +163,14 @@ private object TrainUtils extends Serializable {
     val evalResults: Array[(String, Double)] = state.booster.getEvalResults(state.evalNames, 1)
     val results: Array[(String, Double)] = evalResults.zipWithIndex.map { case ((evalName, evalScore), index) =>
       log.info(s"Valid $evalName=$evalScore")
-      val cmp =
-        if (evalName.startsWith("auc")
-            || evalName.startsWith("ndcg@")
-            || evalName.startsWith("map@")
-            || evalName.startsWith("average_precision"))
-          (x: Double, y: Double, tol: Double) => x - y > tol
-        else
-          (x: Double, y: Double, tol: Double) => x - y < tol
       if (state.bestScores(index) == null
-          || cmp(evalScore, state.bestScore(index), state.ctx.trainingCtx.improvementTolerance)) {
+          || isImprovement(evalName, evalScore, state.bestScore(index),
+            state.ctx.trainingCtx.improvementTolerance)) {
         state.bestScore(index) = evalScore
         state.bestIteration(index) = state.iteration
         state.bestScores(index) = evalResults.map(_._2)
-      } else if (state.iteration - state.bestIteration(index) >= state.ctx.trainingCtx.earlyStoppingRound) {
+      } else if (shouldStopEarly(state.iteration, state.bestIteration(index),
+        state.ctx.trainingCtx.earlyStoppingRound)) {
         state.isFinished = true
         log.info("Early stopping, best iteration is " + state.bestIteration(index))
         state.bestIterationResult = Some(state.bestIteration(index))

lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMParams.scala

@@ -185,13 +185,18 @@ trait LightGBMDatasetParams extends Wrappable {
 /** Defines common parameters across all LightGBM learners related to learning score evolution.
   */
 trait LightGBMLearnerParams extends Wrappable {
-  val earlyStoppingRound = new IntParam(this, "earlyStoppingRound", "Early stopping round")
+  val earlyStoppingRound = new IntParam(this, "earlyStoppingRound",
+    "Number of rounds without sufficient improvement before stopping; zero disables early stopping",
+    ParamValidators.gtEq(0))
   setDefault(earlyStoppingRound -> 0)
   def getEarlyStoppingRound: Int = $(earlyStoppingRound)
   def setEarlyStoppingRound(value: Int): this.type = set(earlyStoppingRound, value)
 
-  val improvementTolerance = new DoubleParam(this, "improvementTolerance",
-    "Tolerance to consider improvement in metric")
+  val improvementTolerance = new DoubleParam(
+    this,
+    "improvementTolerance",
+    "Metric improvement must exceed this value to reset the early stopping counter",
+    ParamValidators.gtEq(0.0))
   setDefault(improvementTolerance -> 0.0)
   def getImprovementTolerance: Double = $(improvementTolerance)
   def setImprovementTolerance(value: Double): this.type = set(improvementTolerance, value)

lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/TrainUtilsSuite.scala

@@ -0,0 +1,96 @@
+// Copyright (C) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License. See LICENSE in project root for information.
+
+package com.microsoft.azure.synapse.ml.lightgbm.split1
+
+import com.microsoft.azure.synapse.ml.lightgbm.{LightGBMRegressor, TrainUtils}
+import org.scalatest.funsuite.AnyFunSuite
+
+class TrainUtilsSuite extends AnyFunSuite {
+
+  private val lowerIsBetterMetrics = Seq(
+    "rmse",
+    "l1",
+    "mae",
+    "l2",
+    "mse",
+    "binary_logloss",
+    "binary_error",
+    "multi_logloss",
+    "multi_error",
+    "mape",
+    "quantile",
+    "huber",
+    "fair",
+    "poisson",
+    "gamma",
+    "gamma_deviance",
+    "tweedie",
+    "cross_entropy",
+    "kullback_leibler")
+
+  private val higherIsBetterMetrics = Seq(
+    "auc",
+    "auc_mu",
+    "ndcg@1",
+    "ndcg@10",
+    "map@1",
+    "map@10",
+    "average_precision")
+
+  private val tolerances = Seq(0.0, 0.25, 2.0, 25.0)
+
+  test("Improvement tolerance is symmetric across metrics and tolerance values") {
+    val bestScore = 100.0
+    val margin = 0.125
+
+    tolerances.foreach { tolerance =>
+      lowerIsBetterMetrics.foreach { metric =>
+        assert(TrainUtils.isImprovement(metric, bestScore - tolerance - margin, bestScore, tolerance))
+        assert(!TrainUtils.isImprovement(metric, bestScore - tolerance, bestScore, tolerance))
+        assert(!TrainUtils.isImprovement(metric, bestScore - tolerance / 2, bestScore, tolerance))
+        assert(!TrainUtils.isImprovement(metric, bestScore + margin, bestScore, tolerance))
+      }
+
+      higherIsBetterMetrics.foreach { metric =>
+        assert(TrainUtils.isImprovement(metric, bestScore + tolerance + margin, bestScore, tolerance))
+        assert(!TrainUtils.isImprovement(metric, bestScore + tolerance, bestScore, tolerance))
+        assert(!TrainUtils.isImprovement(metric, bestScore + tolerance / 2, bestScore, tolerance))
+        assert(!TrainUtils.isImprovement(metric, bestScore - margin, bestScore, tolerance))
+      }
+    }
+  }
+
+  test("Zero early stopping rounds disable wrapper early stopping") {
+    assert(!TrainUtils.shouldStopEarly(
+      iteration = 100,
+      bestIteration = 0,
+      earlyStoppingRound = 0))
+  }
+
+  test("Positive early stopping rounds stop only when the round boundary is reached") {
+    assert(!TrainUtils.shouldStopEarly(iteration = 4, bestIteration = 0, earlyStoppingRound = 5))
+    assert(TrainUtils.shouldStopEarly(iteration = 5, bestIteration = 0, earlyStoppingRound = 5))
+    assert(TrainUtils.shouldStopEarly(iteration = 10, bestIteration = 5, earlyStoppingRound = 5))
+  }
+
+  test("Early stopping parameters accept valid values and reject invalid values") {
+    val learner = new LightGBMRegressor()
+
+    assert(learner.getEarlyStoppingRound == 0)
+    assert(learner.getImprovementTolerance == 0.0)
+
+    Seq(0, 1, 100, Int.MaxValue).foreach { earlyStoppingRound =>
+      assert(learner.setEarlyStoppingRound(earlyStoppingRound).getEarlyStoppingRound == earlyStoppingRound)
+    }
+    assertThrows[IllegalArgumentException](learner.setEarlyStoppingRound(-1))
+
+    Seq(0.0, 0.25, 25.0, Double.MaxValue).foreach { tolerance =>
+      assert(learner.setImprovementTolerance(tolerance).getImprovementTolerance == tolerance)
+    }
+
+    Seq(-0.25, Double.NegativeInfinity, Double.NaN).foreach { tolerance =>
+      assertThrows[IllegalArgumentException](learner.setImprovementTolerance(tolerance))
+    }
+  }
+}