Skip to content

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

Merged
merged 12 commits into from
Jun 24, 2020

Conversation

glemaitre
Copy link
Member

@glemaitre glemaitre commented Jun 11, 2020

closes #17565
partially addressing #15573

Add a pos_label parameter to specify which class to be the positive class when estimator.classes_[1] is not the right choice.

@glemaitre glemaitre changed the title Provide a pos_label parameter in plot_precision_recall_curve EHN Provide a pos_label parameter in plot_precision_recall_curve Jun 11, 2020
thomasjpfan
thomasjpfan previously approved these changes Jun 11, 2020
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.

Thank you @glemaitre ! LGTM

Edit: Dose not work with response_method='decision_function'

@thomasjpfan thomasjpfan dismissed their stale review June 11, 2020 15:46

Testin Failing T_T

@ogrisel
Copy link
Member

ogrisel commented Jun 11, 2020

Should we display the pos label in the axis labels or in the title of the plot?

@thomasjpfan
Copy link
Member

Do we restrict this to response_method='predict_proba'?

@glemaitre
Copy link
Member Author

Should we display the pos label in the axis labels or in the title of the plot?

I think it would be great indeed.

Do we restrict this to response_method='predict_proba'?

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.

@glemaitre
Copy link
Member Author

Basically we will be fine with decision_function since we are giving the y_pred which is 1D and pos_label to the precision_recall_curve function which will manage properly the issue with those.

The probability is more problematic because precision_recall_curve should be a 1D array and we have to do the job before to pass the function.

@glemaitre
Copy link
Member Author

I think that the question of @amueller in #15573 should be addressed. Is pos_label the right parameter name with its slightly semantic being different from the precision_recall_curve for instance.

@thomasjpfan
Copy link
Member

We are using pos_label to do two things here:

  1. Select the positive class from classes_
  2. Passing to precision_recall_curve which selects the positive class from y_true.

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.

@glemaitre
Copy link
Member Author

Fair enough with me. Would we consider this as a bug fix or an enhancement?

@thomasjpfan
Copy link
Member

Adding a new parameter feels like an enhancement.

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.

LGTM

@claramatos
Copy link
Contributor

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

@thomasjpfan
Copy link
Member

This needs a second reviewer to approve before it can get merged.

@claramatos
Copy link
Contributor

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?

@amueller
Copy link
Member

@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.

@glemaitre
Copy link
Member Author

I'll be a bit annoying here. Anybody able to make a second review and a potential merge @ogrisel @jeremiedbb @amueller @NicolasHug

Copy link
Member

@ogrisel ogrisel 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. Just a nit. I will merge when green.

@ogrisel ogrisel merged commit e5b99ea into scikit-learn:master Jun 24, 2020
rubywerman pushed a commit to MLH-Fellowship/scikit-learn that referenced this pull request Jun 24, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plot_precision_recall_curve should expose pos_label
5 participants