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

[srowen]

What changes were proposed in this pull request?

See related discussion at #14643

This actually changes more than what the original JIRA encompassed, but does propose a more reasonable (?) and deterministic result in this and other corner cases.

Revise semantics of ProbabilisticClassifierModel thresholds so that classes can only be predicted if they exceed their threshold (meaning no class may be predicted), and otherwise ordering by highest probability, then lowest threshold, then by class index.

How was this patch tested?

Existing and new unit tests.

[SparkQA]

Test build #64900 has finished for PR 14949 at commit 08dbe43.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #64903 has finished for PR 14949 at commit 2fa331e.

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

[srowen]

@jkbradley @zhengruifeng @MLnick I wonder if I could ask you for comments on this change? it's a behavior change, so not something I'd do lightly, but I do think it improves the semantics here.

[srowen]

Trying @holdenk or @mengxr maybe. I think this behavior should be changed because it doesn't match the common meaning of 'threshold', but I feel like I'm missing context about why it was done this way.

[zhengruifeng]

I think both this change and current design are reasonable. And I personally prefer to current one which treat threshould as a kind of weight.

[srowen]

The problem is that it's called 'threshold' and not 'weight', and 'threshold' means something different. Is anyone suggesting that it was always meant as a 'weight', and/or has a reference for this type of weighting? I've never seen it ... not sure what multiplying a probability by 1/weight would mean theoretically.

Note this change would also make threshold matter as a tie-breaker.

[MLnick]

The original JIRA SPARK-8069 refers to https://cran.r-project.org/web/packages/randomForest/randomForest.pdf.

That R package calls it "cutoff". Though it does indeed seem to act more like a "weight" or "scaling". I can't say I've come across it before, and it appears this is the only package that does it like this (at least that I've been able to find from some quick searching). I haven't found any theoretical background for it either.

In any case, now that we have it, I think it probably best to keep it as is. However, It appears that our implementation here is flawed since in the original R code, the cutoff vector sum must be in (0, 1) (and also be >0 everywhere) - see https://github.com/cran/randomForest/blob/9208176df98d561aba6dae239472be8b124e2631/R/predict.randomForest.R#L47. If we're going to base something on another impl, probably best to actually follow it.

So:

• If sum(thresholds) > 1 or < 0, throw and error
• If each entry in thresholds not > 0, throw an error

I believe this takes care of the edge cases since no thresholds can be 0 or 1. The tie breaker element is taken care of with Vector.argmax (if p/t is the same for 2 or more classes, then ties will effectively be broken by class index order).

I don't like returning NaN. Since the R impl is actually scaling things rather than actually "cutting off" or "thresholding", it should always return a prediction and I think we should too.

[srowen]

Oh, I get it now. That makes sense. If this were being applied to decision trees only, that would make sense and we could fix this up and document the meaning. I agree it only makes sense to return "no class" if actually thresholding.

The only problem here is that this is not being applied just to a random forest implementation but to all classifiers that output a probability. That's a little more of a stretch. I suppose the result here can be thought of as a likelihood ratio of class probability vs prior, not some hacky heuristic specific to the CRAN package. I think the name is unfortunate because I would not have guessed that's the meaning given the name (though to be fair the scaladoc does say what it means).

I'll close this but what's the best way forward?

Option 1.
Keep current behavior. Modify #14643 to include Nick's suggestions above, and add a bunch of documentation about what 'thresholds' really means here.

Option 2.
As above but deprecate threshold and rename to 'cutoff' to be a little clearer.

Option 3.
As in Option 2 but also go back and actually implement thresholds.