[SPARK-17057][ML] ProbabilisticClassifierModels' prediction more reasonable with multi zero thresholds

[zhengruifeng]

What changes were proposed in this pull request?

Change the behavior of transform in ProbabilisticClassifierModel while there are more than one thresholds set zero.

How was this patch tested?

unit tests and manual tests

[SparkQA]

Test build #63774 has finished for PR 14643 at commit 4ec606d.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

You don't need the check for 0.0 in this case right?

I see the logic of the change, but I wonder what the motivation is for computing a ratio of probabilities here to begin with? I wonder if K-L divergence makes sense here instead: p*math.log(p/t) + (1-p)*math.log((1-p)/(1-t))

@holdenk I believe you added this bit in zhengruifeng@5a23213#diff-4a7c1a2a6f2706d045b694f6d7a054f1R201 ?

As you can see that formula would imply that t > 0 and t < 1. t == 0 means "always predict this", and t == 1 means "never predict this". t == 1 is easy enough to filter out from consideration, though maybe args checking should catch the case that all thresholds are 1 earlier on.

t == 0 is valid if it's true of just one class. That too can be checked when it's specified. If that's the case, of course the class to predict is clear. Otherwise K-L applies.

I wonder if it's worth expanding scope to consider this?

[holdenk]

I did add this along with @jkbradley, so this PR says its goal is to make the prediction more reasonable with multiple 0 threshold classes, which I'm wondering if that is a thing that really makes a lot of sense - but I'm certainly open to the idea we could handle thresholds in a different way (although we would certainly want to be careful with any such changes).

[srowen]

Yeah, there are maybe four closely-related issues.

• what to do with t = 0 in more than one class, and it should probably (?) be an error to even specify this
• what about t = 1, especially the case that all are 1?
• what about the case where nothing exceeds a threshold at all?
• is the ratio computation the best way to choose among several classes that exceed the threshold?

K-L divergence actually gives a different answer. It would rank p=0.5/t=0.2 higher than p=0.3/t=0.1, whereas the current rule is the reverse. I think K-L is more theoretically sound (though someone may tell me there's an equivalent or simpler way to think of it). However that is the most separable question of the 4.

[srowen]

Here's an alternative take on the method that I think addresses all of these:

  protected def probability2prediction(probability: Vector): Double = {
    if (!isDefined(thresholds)) {
      return probability.argmax
    }

    val probArray = probability.toArray
    val candidates = probArray.zip(getThresholds).filter { case (p, t) => p >= t }
    if (candidates.isEmpty) {
      return Double.NaN
    }

    // The threshold t notionally defines a Bernoulli distribution, as does p.
    // A measure of the degree to which p "exceeds" t is the distance between these
    // distributions, and this is the Kullback-Leibler divergence:
    // p ln(p/t) + (1-p )ln((1-p)/(1-t))
    // We assume p >= t from above. Pick the p,t whose KL divergence is highest, taking
    // care to deal with p=0/1 and t=0/1 carefully.

    // Computes p ln(p/t) even when p = 0
    def plnpt(p: Double, t: Double) = if (p == 0.0) 0.0 else p * math.log(p / t)

    val klDivergences = candidates.map {
      case (p, t) if p == t => 0.0 // because distributions match
      case (p, t) if t == 0.0 => Double.PositiveInfinity // because p > 0
      case (p, t) => plnpt(p, t) + plnpt(1.0 - p, 1.0 - t)
    }

    val maxKL = klDivergences.max
    val klAndIndex = klDivergences.zipWithIndex.filter { case (kl, _) => kl == maxKL }
    if (klAndIndex.length == 1) {
      // One max? return its index
      val (_, maxIndex) = klAndIndex.head
      maxIndex
    } else {
      // Many max? break tie by conditional probability
      val ((_, maxIndex), _) = klAndIndex.zip(probArray).maxBy { case (_, p) => p }
      maxIndex
    }
  }

The significant changes are a slight change in behavior, but also correctly handling the case where no thresholds are exceeded.

really this method should return Option[Int], but the API is Double, so the result in this case is NaN. Not ideal but kind of stuck with it.

[srowen]

Or, a simpler take is this: if thresholds are just meant to provide a minimum class probability, then the logic should simply be to choose the class with the highest probability that also exceeds its threshold. What about that?

[zhengruifeng]

@srowen I though of threshoulds designed in ML just as a kind of weight. This design is easy to understand. Is there some other librarys (like sklearn) that support thresholds? We can refer to it.

[srowen]

Thresholds are just that -- thresholds. The meaning is certainly as in #14643 (comment) While I kind of like the idea of also treating them as a 'weight', I hadn't seen that before. Maybe it's best to undo this meaning if there's no reference for it. @jkbradley do you know anything about the history here, per #14643 (comment)

[srowen]

If it's OK, I'll open a different PR which proposes a simpler behavior:

• Return the class with highest probability that is also >= threshold
• If no such class exists, return ... NaN? This case really must be handled but I don't see it was designed to be here
• If more than one such class exists, choose the one with lowest threshold
• If more than one such class exists, pick the first one

[srowen]

... for example,

  /**
   * Given a vector of class conditional probabilities, select the predicted label.
   * This returns the class, if any, whose probability is equal to or greater than its
   * threshold (if specified), and whose probability is highest. If several classes meet
   * their thresholds and are equally probable, the one with lower threshold is selected.
   * If several have equal thresholds, the one with lower class index is selected.
   * @return  predicted label
   */
  protected def probability2prediction(probability: Vector): Double = {
    if (isDefined(thresholds)) {
      probability.toArray.zip(getThresholds).zipWithIndex.
        filter { case ((p, t), _) => p >= t }.
        maxBy { case ((p, t), i) => (p, -t, -i) }._2
    } else {
      probability.toArray.zipWithIndex.maxBy { case (p, i) => (p, -i) }._2
    }
  }

@jkbradley what do you think? this is entirely deterministic now, interprets thresholds in the more usual way, but still uses them to break ties.

The only catch here is that this tries to explicitly handle the case where nothing exceeds its threshold now. It will now return NaN rather than actually return a class whose probability doesn't meet its threshold.

[srowen]

See #14949 -- I think we might want to proceed with this with some modifications.

[srowen]

Ping @zhengruifeng are you in a position to keep working on this or should I take it over?

[zhengruifeng]

@srowen You can take it over.

[srowen]

OK, we can close this PR then.