Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zhengruifeng wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions mllib/src/main/scala/org/apache/spark/ml/classification/ProbabilisticClassifier.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,11 +201,18 @@ abstract class ProbabilisticClassificationModel[
probability.argmax
} else {
val thresholds: Array[Double] = getThresholds
val scaledProbability: Array[Double] =
probability.toArray.zip(thresholds).map { case (p, t) =>
if (t == 0.0) Double.PositiveInfinity else p / t
}
Vectors.dense(scaledProbability).argmax

if (thresholds.contains(0.0)) {
val indices = thresholds.zipWithIndex.filter(_._1 == 0.0).map(_._2)
val values = indices.map(probability.apply)
Vectors.sparse(numClasses, indices, values).argmax
} else {
val scaledProbability: Array[Double] =
probability.toArray.zip(thresholds).map { case (p, t) =>
if (t == 0.0) Double.PositiveInfinity else p / t
Copy link
Member

@srowen srowen Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@holdenk holdenk Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@srowen srowen Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@srowen srowen Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@srowen srowen Aug 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
Vectors.dense(scaledProbability).argmax
}
}
}
}
Expand Down