Skip to content

FIX pos_label constraint in roc_curve (param validation) #25131

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

Jump to bottom
Merged
merged 2 commits into from
Dec 7, 2022
Merged

FIX pos_label constraint in roc_curve (param validation) #25131

merged 2 commits into from
Dec 7, 2022

Conversation

jeremiedbb
Copy link
Member

glemaitre
glemaitre reviewed Dec 7, 2022
View reviewed changes
sklearn/metrics/_ranking.py Outdated
@@ -723,7 +723,7 @@ def _binary_clf_curve(y_true, y_score, pos_label=None, sample_weight=None):
y_score : ndarray of shape (n_samples,)
Estimated probabilities or output of a decision function.

pos_label : int or str, default=None
pos_label : int, float or str, default=None
Copy link
Member

Choose a reason for hiding this comment

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

Do we add bool as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

right, I forgot

thomasjpfan
thomasjpfan approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

I do not really like how floats can be a classification target, but it is what we support according to type_of_target:

from sklearn.utils.multiclass import type_of_target
import numpy as np

X = np.asarray([1.0, 2.0, 1.0, 2.0])
type_of_target(X)
# 'binary'

As for this PR, LGTM

glemaitre
glemaitre approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM as well. Yep float is surprising but I would not be surprised that we have use cases with them

@jeremiedbb
Copy link
Member Author

Merging with 2 approvals

@jeremiedbb jeremiedbb merged commit 929b3dc into scikit-learn:main Dec 7, 2022
Vincent-Maladiere pushed a commit to Vincent-Maladiere/scikit-learn that referenced this pull request Dec 14, 2022
@jeremiedbb @Vincent-Maladiere
FIX pos_label constraint in roc_curve (param validation) (scikit-lear…
177b794 
…n#25131)
@jjerphan jjerphan added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Jan 19, 2023
@jjerphan jjerphan added this to the 1.2.1 milestone Jan 19, 2023
@jjerphan jjerphan mentioned this pull request Jan 19, 2023
Merged
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@jeremiedbb @jjerphan
FIX pos_label constraint in roc_curve (param validation) (scikit-lear…
eeaa02b 
…n#25131)
@jjerphan jjerphan mentioned this pull request Jan 20, 2023
Merged
12 tasks
@jjerphan jjerphan modified the milestones: 1.2.1, 1.3 Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@glemaitre glemaitre glemaitre approved these changes

Assignees
No one assigned
Labels
module:metrics To backport PR merged in master that need a backport to a release branch defined based on the milestone.
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.
4 participants
@jeremiedbb @thomasjpfan @glemaitre @jjerphan