fix: correct Long hyperparameter sampling, Float seed forwarding, form validation, and Jaccard similarity

cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala

@@ -12,6 +12,33 @@ import com.microsoft.azure.synapse.ml.stages.DropColumns
 class ModelEqualitySuite extends TestBase {
   spark
 
+  test("jaccardSimilarity grades partial overlap") {
+    // Sets of whole strings would collapse this to a 1.0/0.0 equality check.
+    assert(ModelEquality.jaccardSimilarity("abcd", "abcd") === 1.0)
+    assert(ModelEquality.jaccardSimilarity("abcd", "wxyz") === 0.0)
+    val partial = ModelEquality.jaccardSimilarity("the quick brown fox", "the quick brown cat")
+    assert(partial > 0.0 && partial < 1.0)
+    assert(partial > ModelEquality.jaccardSimilarity("the quick brown fox", "entirely unlike"))
+  }
+
+  test("jaccardSimilarity is case insensitive and symmetric") {
+    assert(ModelEquality.jaccardSimilarity("Hello World", "hello world") === 1.0)
+    assert(ModelEquality.jaccardSimilarity("kitten", "sitting")
+      === ModelEquality.jaccardSimilarity("sitting", "kitten"))
+    assert(ModelEquality.jaccardSimilarity("", "") === 1.0)
+  }
+
+  test("jaccardSimilarity handles strings shorter than one bigram") {
+    // sliding(2) keeps the short final window, so a 1-char string yields Set(char)
+    // rather than the empty set. Distinct 1-char strings must therefore score 0.0,
+    // not fall into the both-empty shortcut.
+    assert(ModelEquality.jaccardSimilarity("a", "b") === 0.0)
+    assert(ModelEquality.jaccardSimilarity("a", "a") === 1.0)
+    assert(ModelEquality.jaccardSimilarity("", "a") === 0.0)
+    assert(ModelEquality.jaccardSimilarity("a", "") === 0.0)
+    assert(ModelEquality.jaccardSimilarity("", "") === 1.0)
+  }
+
   test("Complex param equality") {
     val m1 = new TextSentiment().setLocation("eastus")
     val m2 = new TextSentiment().setLocation("eastus")

core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala

@@ -5,6 +5,7 @@ package com.microsoft.azure.synapse.ml.automl
 
 import org.apache.spark.ml.param._
 
+import scala.annotation.tailrec
 import scala.collection.JavaConverters._
 import scala.collection.mutable
 import scala.util.Random
@@ -29,16 +30,48 @@ class LongRangeHyperParam(min: Long, max: Long, seed: Long = 0)
     extends RangeHyperParam[Long](min, max, seed) {
 
   def getNext(): Long = {
+    require(max >= min, s"LongRangeHyperParam requires max >= min, got min=$min, max=$max")
     val range = max - min
-    (random.nextLong() * range) + min
+    // nextLong() spans the full 64-bit range, so multiplying it by the range overflows and
+    // escapes [min, max) entirely. Reduce into the range first, as nextInt(range) does.
+    if (range == 0) {
+      min
+    } else if (range < 0) {
+      // The span is wider than Long.MaxValue so it cannot be represented as a Long. Draw from
+      // the whole 64-bit range instead; more than half of it lands in bounds, so this is cheap.
+      @tailrec def drawInBounds(): Long = {
+        val v = random.nextLong()
+        if (v >= min && v < max) v else drawInBounds()
+      }
+      drawInBounds()
+    } else {
+      boundedNextLong(range) + min
+    }
+  }
+
+  /** Uniform draw from [0, bound), rejecting the partial final block so the result carries no
+    * modulo bias. Mirrors what java.util.Random.nextInt(bound) already does for Int.
+    */
+  private def boundedNextLong(bound: Long): Long = {
+    val m = bound - 1
+    if ((bound & m) == 0L) {
+      random.nextLong() & m // bound is a power of two, so the low bits are already uniform
+    } else {
+      @tailrec def draw(): Long = {
+        val u = random.nextLong() >>> 1
+        val r = u % bound
+        if (u + m - r < 0L) draw() else r
+      }
+      draw()
+    }
   }
 
 }
 
 class FloatRangeHyperParam(min: Float, max: Float, seed: Long = 0)
     extends RangeHyperParam[Float](min, max, seed) {
 
-  val doubleRange = new DoubleRangeHyperParam(min.toDouble, max.toDouble)
+  val doubleRange = new DoubleRangeHyperParam(min.toDouble, max.toDouble, seed)
   def getNext(): Float = {
     doubleRange.getNext().toFloat
   }

core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala

@@ -14,10 +14,13 @@ trait ParamEquality[T] extends Param[T] {
 
 object ModelEquality {
 
+  /** Similarity of two strings as the Jaccard index over their character bigrams. */
   def jaccardSimilarity(s1: String, s2: String): Double = {
-    val a = Set(s1)
-    val b = Set(s2)
-    a.intersect(b).size.toDouble / (a | b).size.toDouble
+    // Sets of the whole strings would only ever yield 1.0 or 0.0, making this a strict
+    // equality check rather than a similarity measure.
+    val a = s1.toLowerCase.sliding(2).toSet
+    val b = s2.toLowerCase.sliding(2).toSet
+    if (a.isEmpty && b.isEmpty) 1.0 else a.intersect(b).size.toDouble / (a | b).size.toDouble
   }
 
   def assertEqual(m1: Params, m2: Params): Unit = {

core/src/main/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalize.scala

@@ -32,8 +32,9 @@ class UnicodeNormalize(val uid: String) extends Transformer
 
   /** @group setParam */
   def setForm(value: String): this.type = {
-    // check input value
-    Normalizer.Form.valueOf(getForm)
+    // Validate the incoming value, not the value already set. Validating getForm let an invalid
+    // form be stored and only surface later, inside the UDF on the executors.
+    Normalizer.Form.valueOf(value)
 
     set("form", value)
   }

core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala

@@ -0,0 +1,99 @@
+// 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.automl
+
+import com.microsoft.azure.synapse.ml.core.test.base.TestBase
+
+// scalastyle:off magic.number
+/** Covers the bounds and seeding contract shared by every RangeHyperParam. */
+class HyperparamRangeSuite extends TestBase {
+
+  private val draws = 500
+
+  test("LongRangeHyperParam stays within its range") {
+    val hp = new LongRangeHyperParam(0L, 100L, seed = 42)
+    val values = (1 to draws).map(_ => hp.getNext())
+    assert(values.forall(v => v >= 0L && v < 100L), s"out of range: ${values.filter(v => v < 0L || v >= 100L)}")
+    assert(values.toSet.size > 1)
+  }
+
+  test("LongRangeHyperParam stays within a range too large to fit in an Int") {
+    val min = -4000000000L
+    val max = 4000000000L
+    val hp = new LongRangeHyperParam(min, max, seed = 7)
+    val values = (1 to draws).map(_ => hp.getNext())
+    assert(values.forall(v => v >= min && v < max))
+    assert(values.exists(_ < 0L) && values.exists(_ > 0L))
+  }
+
+  test("LongRangeHyperParam rejects an inverted range instead of silently returning min") {
+    val hp = new LongRangeHyperParam(10L, 5L, seed = 1)
+    assertThrows[IllegalArgumentException](hp.getNext())
+  }
+
+  test("LongRangeHyperParam stays in bounds when the span overflows Long") {
+    // max - min wraps negative here, so the ordinary reduction path cannot be used.
+    val hp = new LongRangeHyperParam(Long.MinValue + 1, Long.MaxValue, seed = 7)
+    val draws = (1 to 200).map(_ => hp.getNext())
+    assert(draws.forall(v => v >= Long.MinValue + 1 && v < Long.MaxValue))
+    assert(draws.distinct.length > 1)
+  }
+
+  test("LongRangeHyperParam with an empty range returns min") {
+    val hp = new LongRangeHyperParam(5L, 5L, seed = 42)
+    assert((1 to 10).forall(_ => hp.getNext() === 5L))
+  }
+
+  test("LongRangeHyperParam covers a power-of-two range without bias") {
+    // Power-of-two bounds take the mask branch; every bucket must still be reachable.
+    val hp = new LongRangeHyperParam(0L, 8L, seed = 3)
+    val counts = (1 to 4000).map(_ => hp.getNext()).groupBy(identity).map { case (k, v) => k -> v.size }
+    assert(counts.keySet === (0L until 8L).toSet)
+    assert(counts.values.forall(c => c > 300 && c < 700), s"uneven distribution: $counts")
+  }
+
+  test("LongRangeHyperParam covers a non-power-of-two range without bias") {
+    // Non-power-of-two bounds take the rejection branch, which must terminate and stay uniform.
+    val hp = new LongRangeHyperParam(0L, 7L, seed = 3)
+    val counts = (1 to 4000).map(_ => hp.getNext()).groupBy(identity).map { case (k, v) => k -> v.size }
+    assert(counts.keySet === (0L until 7L).toSet)
+    assert(counts.values.forall(c => c > 350 && c < 800), s"uneven distribution: $counts")
+  }
+
+  test("IntRangeHyperParam stays within its range") {
+    val hp = new IntRangeHyperParam(5, 15, seed = 42)
+    val values = (1 to draws).map(_ => hp.getNext())
+    assert(values.forall(v => v >= 5 && v < 15))
+  }
+
+  test("DoubleRangeHyperParam stays within its range") {
+    val hp = new DoubleRangeHyperParam(0.0, 1.0, seed = 42)
+    assert((1 to draws).map(_ => hp.getNext()).forall(v => v >= 0.0 && v < 1.0))
+  }
+
+  test("FloatRangeHyperParam stays within its range") {
+    val hp = new FloatRangeHyperParam(0.0f, 1.0f, seed = 42)
+    assert((1 to draws).map(_ => hp.getNext()).forall(v => v >= 0.0f && v < 1.0f))
+  }
+
+  test("equal seeds reproduce equal sequences and differing seeds diverge") {
+    def draw(hp: Dist[_]): Seq[Any] = (1 to 20).map(_ => hp.getNext)
+
+    assert(draw(new IntRangeHyperParam(0, 1000, 11)) === draw(new IntRangeHyperParam(0, 1000, 11)))
+    assert(draw(new LongRangeHyperParam(0L, 1000L, 11)) === draw(new LongRangeHyperParam(0L, 1000L, 11)))
+    assert(draw(new DoubleRangeHyperParam(0.0, 1.0, 11)) === draw(new DoubleRangeHyperParam(0.0, 1.0, 11)))
+    // FloatRangeHyperParam delegates to an inner DoubleRangeHyperParam; it must forward its seed.
+    assert(draw(new FloatRangeHyperParam(0.0f, 1.0f, 11)) === draw(new FloatRangeHyperParam(0.0f, 1.0f, 11)))
+    assert(draw(new FloatRangeHyperParam(0.0f, 1.0f, 11)) !== draw(new FloatRangeHyperParam(0.0f, 1.0f, 12)))
+  }
+
+  test("HyperParamUtils.getRangeHyperParam honors the seed for every numeric type") {
+    Seq[(Any, Any)]((0, 1000), (0L, 1000L), (0.0, 1.0), (0.0f, 1.0f)).foreach { case (min, max) =>
+      val a = HyperParamUtils.getRangeHyperParam(min, max, 99)
+      val b = HyperParamUtils.getRangeHyperParam(min, max, 99)
+      assert((1 to 20).map(_ => a.getNext) === (1 to 20).map(_ => b.getNext), s"seed ignored for $min/$max")
+    }
+  }
+}
+// scalastyle:on magic.number

core/src/test/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalizeSuite.scala

@@ -5,6 +5,7 @@ package com.microsoft.azure.synapse.ml.stages
 
 import com.microsoft.azure.synapse.ml.core.test.base.TestBase
 import com.microsoft.azure.synapse.ml.core.test.fuzzing.{TestObject, TransformerFuzzing}
+import org.apache.spark.ml.param.Param
 import org.apache.spark.ml.util.MLReadable
 import org.apache.spark.sql.DataFrame
 
@@ -49,9 +50,25 @@ class UnicodeNormalizeSuite extends TestBase with TransformerFuzzing[UnicodeNorm
 
   test("Check for NFKD forms") { testForm("NFKD", expectedResultDecomposed) }
 
+  test("setForm rejects an invalid form at set time") {
+    // An unvalidated form used to be accepted here and only fail later inside the UDF,
+    // on the executors, where the cause is far harder to attribute.
+    assertThrows[IllegalArgumentException](new UnicodeNormalize().setForm("NOT_A_FORM"))
+  }
+
+  test("setForm accepts every supported form") {
+    java.text.Normalizer.Form.values().foreach { f =>
+      assert(new UnicodeNormalize().setForm(f.name).getForm === f.name)
+    }
+  }
+
   def testObjects(): Seq[TestObject[UnicodeNormalize]] = List(new TestObject(
     new UnicodeNormalize().setInputCol("words").setOutputCol("out"), makeBasicDF()))
 
+  // The generic fuzzer probes String setters with "foo"; form only accepts Normalizer.Form names.
+  override def getterSetterParamExamples(pipelineStage: UnicodeNormalize): Map[Param[_], Any] =
+    Map[Param[_], Any]((pipelineStage.form, "NFC"))
+
   override def reader: MLReadable[_] = UnicodeNormalize
 
 }