FIX Fixes OneVsOneClassifier.predict for Estimators with only predict_proba #22604
Conversation
thomasjpfan
added
module:multiclass
Quick Review
|
Marking this as quick review. We do a very similar thing here: scikit-learn/sklearn/multiclass.py Lines 429 to 434 in 70eebb9 |
thomasjpfan
changed the title
sklearn/multiclass.py
Outdated
| if hasattr(self.estimators_[0], "decision_function") and is_classifier( | ||
| self.estimators_[0] | ||
| ): | ||
| thresh = 0 | ||
| else: | ||
| # predict_proba threshold | ||
| thresh = 0.5 | ||
| return self.classes_[(Y > thresh).astype(int)] |
There was a problem hiding this comment.
Is it worth wrapping this and the lines you mentioned, i.e.:
scikit-learn/sklearn/multiclass.py
Lines 429 to 434 in 70eebb9
in a private _threshold property?
I think this can make the logic explicit and ease maintainability.
There was a problem hiding this comment.
OneVsOneClassifier and OneVsRestClassifier do not share a common base class where a _threshold property makes sense. Also the threshold is only used in the binary case.
Inspired by your suggestion, I think a helper function makes the logic better 7372cee (#22604)
|
Merging main to get rid of the doc-min-dependencies error about Python 3.7. |
|
Merging, thanks! |
…_proba (scikit-learn#22604) Co-authored-by: Loïc Estève <[email protected]>
Reference Issues/PRs
Fixes #13617
What does this implement/fix? Explain your changes.
This PR uses the correct threshold if the inner estimator uses
predict_proba.