-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MNT Refactor scorer using _get_response #18589
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.
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
Conversation
f"pos_label={pos_label} is not a valid label: {classes}" | ||
) | ||
|
||
def _select_proba_binary(self, y_pred, classes): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was moved within _get_response
when selecting the binary probability.
cc27a27
to
a212a81
Compare
@ogrisel @thomasjpfan @NicolasHug I think this is ready for a review. |
I added a summary of what was done in the welcoming message of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @glemaitre , just gave it a quick first look
OK I think this is good for a new review @NicolasHug @ogrisel |
"""Call estimator with method and args and kwargs.""" | ||
response_method = kwargs["response_method"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can update the scorers in a follow up PR so this PR can be a little shorter?
sklearn/utils/validation.py
Outdated
Parameters | ||
---------- | ||
estimator : estimator instance | ||
Classifier to check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used for regressors and classifiers.
There's been no activity here since February when you added this to 1.0 @glemaitre . Do you think this can be merged in time for the release? |
I think it is best to have all plotting display in and then do the refactoring. The refactoring is not a blocker, this is just maintenance.
Sent from my phone - sorry to be brief and potential misspell.
|
this pr is superseded. I'm removing it from the milestone and adding the more recent one instead |
Since this PR is superseded, can we close it? It would be easier to figure out which one is the "current one". |
Superseded by #20999. |
Supersede #18212
Refactor the scorer such that they make use of
_get_response
already define in the plotting function.This refactoring can also be beneficial for #16525.
Summary of what was done:
_check_response_method
. Its job is to return the method of a classifier or a regressor to later predict. If the method does not exist, it raises an error. This function was already existing indeed._get_response
. A function that returns the prediction depending on the response method. We take into account thepos_label
. Thus, it will allow to not make any mistake in the future by forgetting to inverse the decision function or select the right column of probabilities for binary classification. We hard-coded this behaviour in a lot of different places and this function reduces the amount of redundant code.