FEA Adds decision_threshold_curve function
#31338
Conversation
…/scikit-learn into metric_threshold_curve
| labels: array-like, default=None | ||
| Class labels. If `None`, inferred from `y_true`. | ||
|
|
||
| pos_label : int, float, bool or str, default=None |
There was a problem hiding this comment.
The other main question is whether we need to allow the user to set this, even when scoring_function does not take a pos_label parameter?
I think we should have this, because pos_label is passed to _threshold_scores_to_class_labels, and a user should be able to control this.
| @@ -1,6 +1,6 @@ | |||
| .. currentmodule:: sklearn.model_selection | |||
|
|
|||
| .. _TunedThresholdClassifierCV: | |||
| .. _threshold_tunning: | |||
There was a problem hiding this comment.
I referenced this page starting here for decision_threshold_curve because I thought this first paragraph was appropriate, for context, not just the section I added. Not 100% on this though and happy to change
| @@ -1,6 +1,6 @@ | |||
| .. currentmodule:: sklearn.model_selection | |||
|
|
|||
| .. _TunedThresholdClassifierCV: | |||
| .. _threshold_tunning: | |||
|
|
|||
| ================================================== | |||
| Tuning the decision threshold for class prediction | |||
There was a problem hiding this comment.
@glemaitre Can't comment where this is relevant (after L49), but I wonder if it would be interesting to add another scenario where threshold tunning may be of interest - imbalanced datasets?
| good, or a loss function, meaning low is good. In the latter case, the | ||
| the output of `score_func` will be sign-flipped. | ||
|
|
||
| labels : array-like, default=None |
There was a problem hiding this comment.
_CurveScorer uses the term "classes" but "labels" is consistent with what is used for other classification metrics, so chose this.
| between the minimum and maximum of `y_score`. If an array-like, it will be | ||
| used as the thresholds. | ||
|
|
||
| greater_is_better : bool, default=True |
There was a problem hiding this comment.
Again, this is consistent, with term used in other metrics, so avoided using "sign".
| # This could also be done in `decision_threshold_curve`, not sure which | ||
| # is better | ||
| y_true_unique = cached_unique(y_true) | ||
| if classes is None: | ||
| classes = y_true_unique | ||
| # not sure if this separate error msg needed. | ||
| # there is the possibility that set(classes) != set(y_true_unique) fails | ||
| # because `y_true` only contains one class. | ||
| if len(y_true_unique) == 1: | ||
| raise ValueError("`y_true` only contains one class label.") | ||
| if set(classes) != set(y_true_unique): | ||
| raise ValueError( | ||
| f"`classes` ({classes}) is not equal to the unique values found in " | ||
| f"`y_true` ({y_true_unique})." | ||
| ) |
There was a problem hiding this comment.
These checks could be done in decision_threshold_curve instead, not sure which is better.
| for th in potential_thresholds | ||
| ] | ||
| return np.array(score_thresholds), potential_thresholds | ||
| # why 'potential' ? |
There was a problem hiding this comment.
Just for my education, why use the term "potential", in "potential_thresholds". Is it because there a possibility that a threshold is redundant because the predicted labels are the same for adjacent thresholds?
|
@glemaitre I've highlighted questions in the code. Tests still to be added for |
Reference Issues/PRs
closes #25639 (supercedes)
closes #21391
Thought it would be easier to open a new PR and there were no long discussions on the old PR (#25639).
What does this implement/fix? Explain your changes.
Adds a function that takes a scoring function,
y_true,y_score,thresholdsand outputs score per threshold.The intention is to later add a new display class using this function, allowing us to plot metric score per threshold e.g.
Uses the
_CurveScoreras suggested by @glemaitre . Refactors out a new_scores_from_predictionsstatic method, that takes the predictions. The old_scoretakes estimator, calculatesy_scoreand passes to the new_scores_from_prediction.Notes:
_scores_from_predictions- name is inspired by the display class methods 'from_predictions' , but happy to changefrom_scorerbecause we are not using ascorer(which has the signaturecallable(estimator, X, y)) we are using a 'scoring_functionwith signaturescore_func(y_true, y_pred, **kwargs)`_CurveScorerdirectly instead, and then call_scores_from_predictionsindecision_threshold_curvedecided to make[realised that method is nicer to avoid having too many params in_scores_from_predictionsa static method, but I also could have made it a method, and indecision_threshold_curveinstantiate_CurveScorerdirectly first (not viafrom_scorer). Only went with staticmethod path because I initially didn't registerfrom_scorerinstantiates differently than directly via_CurveScorer. Not 100% on what is best here.decision_threshold_curveand avoids some lines of code (use self.xx directly, instead of passing self.xx to_scores_from_predictions)]Any other comments?
cc @glemaitre
Lots more to do, but should get implementation right first.
To do:
_decision_threshold.pymodule docstring