-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
EHN Provide a pos_label parameter in plot_precision_recall_curve #17569
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
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.
Thank you @glemaitre ! LGTM
Edit: Dose not work with response_method='decision_function'
Should we display the pos label in the axis labels or in the title of the plot? |
Do we restrict this to |
I think it would be great indeed.
I did not think yet at the decision function but I think that we should come with a fix as well. I am looking at it now. |
Basically we will be fine with The probability is more problematic because |
We are using
So in the sense of 2, we have the same semantics. I think the semantics for 1 is clear enough compared to the cost of introducing another parameter name. |
Fair enough with me. Would we consider this as a bug fix or an enhancement? |
Adding a new parameter feels like an enhancement. |
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.
LGTM
is this ready to be closed? if so can someone please close it? (@thomasjpfan) I'm working on #15573 and would like to make my changes on top of these ones |
This needs a second reviewer to approve before it can get merged. |
so what is the best thing to do? should I merge this branch into mine? or should I make my changes and then solve possible conflicts once this branch is approved? |
@claramatos the second indeed. Just make your changes against master for now and later resolve conflicts. You could also base your PR on this one but github doesn't have good PR dependency tracking, so this is a bit hard to disentangle. |
I'll be a bit annoying here. Anybody able to make a second review and a potential merge @ogrisel @jeremiedbb @amueller @NicolasHug |
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.
LGTM as well. Just a nit. I will merge when green.
closes #17565
partially addressing #15573
Add a
pos_label
parameter to specify which class to be the positive class whenestimator.classes_[1]
is not the right choice.