[MRG] EHN Provide a pos_label parameter in plot_roc_curve by claramatos · Pull Request #17651 · scikit-learn/scikit-learn

claramatos · 2020-06-21T11:07:53Z

closes #15573
relates with #17569

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

glemaitre

Having the refactoring is nice. I would like to have the other 2 PRs in first thought.
Could you add en entry in whats new in 0.24

sklearn/metrics/_plot/base.py

sklearn/metrics/_plot/precision_recall_curve.py

sklearn/metrics/_plot/tests/test_plot_roc_curve.py

thomasjpfan

I think the refactor of _get_target_scores is nice.

I am +0 on the refactor into a new base class. Is there another visualization we would like to add that would use this new base class?

thomasjpfan · 2020-06-23T05:20:50Z

sklearn/metrics/_plot/base.py

+    estimator_name : str, default=None
+        Name of estimator. If None, then the estimator name is not shown.
+
+    pos_label : str or int, default=None


Having pos_label here makes this base class less generic. We would only be able to use this to plot curves related to classification.

are there any plot classes you believe should use the CurveDisplay class?

@thomasjpfan Do we have curves for regression?
I assume that we could make a trick to find what type of data we handle since we have y and to know if this is a classification problem. If this is not, we can discard pos_label

~~I'll be happy with renaming it to ClassificationCurveDisplay~~

I forgot I had this comment still here.

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

…into is/15573

glemaitre · 2020-06-24T13:27:02Z

@claramatos The 2 other PRs have been merged so we can go forward here. You should resolve the conflict and once done, I will look at the tests.

ogrisel · 2020-06-24T17:09:06Z

Sorry I had to revert #17594 that I merged too quickly (see the discussion at the end for the details).

claramatos · 2020-06-27T17:58:46Z

Since #17569 is closed can this be closed to? (I've already merged the most recent master version) @glemaitre

thomasjpfan

I think PR is doing three things.

Refactor into CurveDisplay, which I am concerned with. I am thinking of how this plays out when we want to support multiclass with many curves. I didn't do this type of refactor at first because I wanted the ROC and PR curve to evolve independently of each other.
Refactor into _get_target_scores, which I am happy with.
Add pos_label into plot_roc_curve, which I am happy with.

@claramatos Can we make this PR only about 2 and 3 and have 1 for a follow up PR?

thomasjpfan · 2020-06-27T18:21:03Z

sklearn/metrics/_plot/base.py

+    estimator_name : str, default=None
+        Name of estimator. If None, then the estimator name is not shown.
+
+    pos_label : str or int, default=None


~~I'll be happy with renaming it to ClassificationCurveDisplay~~

I forgot I had this comment still here.

claramatos · 2020-06-27T18:45:07Z

So your suggestion would be to remove the CurveDisplay refactor from this PR? or renaming CurveDisplay to ClassificationCurveDisplay?

thomasjpfan · 2020-06-27T18:52:44Z

Sorry for being unclear. I am suggesting to remove CurveDisplay and move forward with pos_label + _get_target_scores. After pos_label + _get_target_scores get merged we can do another PR for the CurveDisplay refactor.

claramatos · 2020-06-28T14:41:36Z

@thomasjpfan I followed your suggestion and removed CurveDisplay

thomasjpfan · 2020-06-30T17:49:32Z

sklearn/metrics/_plot/base.py

    return prediction_method
+
+
+def _get_target_scores(X, estimator, response_method, pos_label=None):


Using the word "scores" here may get confused with metrics and scorers. Maybe _get_response is more clear?

sklearn/metrics/_plot/base.py

sklearn/metrics/_plot/precision_recall_curve.py

sklearn/metrics/_plot/tests/test_plot_roc_curve.py

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan

LGTM Thank you @claramatos !

glemaitre · 2020-07-06T14:36:54Z

Actually this is fine since we don't compute the ROC-AUC via roc_auc_score but using auc directly.
We can merge this PR now.

Thanks @claramatos

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

claramatos added 2 commits June 20, 2020 17:06

- move common code to base.py

0bd278f

update comments

60e9ad8

github-actions bot added the module:metrics label Jun 21, 2020

claramatos added 3 commits June 21, 2020 12:27

fix failing lint

a71fdd5

rearrange imports

7f0dd6c

Merge branch 'master' into is/15573

16043ee

glemaitre reviewed Jun 22, 2020

View reviewed changes

thomasjpfan reviewed Jun 23, 2020

View reviewed changes

claramatos and others added 7 commits June 23, 2020 10:05

Update sklearn/metrics/_plot/base.py

f655a27

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Update sklearn/metrics/_plot/tests/test_plot_roc_curve.py

be5aba0

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Update sklearn/metrics/_plot/tests/test_plot_roc_curve.py

f254bc2

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

add cr changes

fdc12df

Merge branch 'is/15573' of https://github.com/claramatos/scikit-learn …

e4018dc

…into is/15573

fix failing test test_plot_roc_curve_pos_label

054b397

update v0.24.rst

5788677

Merge branch 'master' into is/15573

9a195c3

Merge branch 'master' into is/15573

7040107

thomasjpfan reviewed Jun 27, 2020

View reviewed changes

removed CurveDisplay

40e1021

thomasjpfan reviewed Jun 30, 2020

View reviewed changes

claramatos and others added 4 commits June 30, 2020 22:30

Update sklearn/metrics/_plot/precision_recall_curve.py

2f23215

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Update sklearn/metrics/_plot/base.py

bc4c89f

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Update sklearn/metrics/_plot/tests/test_plot_roc_curve.py

eb2f705

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

renaming _get_target_scores to _get_response

0abc323

thomasjpfan approved these changes Jul 4, 2020

View reviewed changes

Merge branch 'master' into is/15573

0657133

glemaitre merged commit c3f2516 into scikit-learn:master Jul 6, 2020

glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Jul 17, 2020

EHN Provide a pos_label parameter in plot_roc_curve (scikit-learn#17651)

410976d

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

EHN Provide a pos_label parameter in plot_roc_curve (scikit-learn#17651)

eb683e3

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

		return prediction_method


		def _get_target_scores(X, estimator, response_method, pos_label=None):

Uh oh!

Conversation

claramatos commented Jun 21, 2020

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

claramatos Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

glemaitre Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Jun 24, 2020

Uh oh!

ogrisel commented Jun 24, 2020

Uh oh!

claramatos commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claramatos commented Jun 27, 2020

Uh oh!

thomasjpfan commented Jun 27, 2020

Uh oh!

claramatos commented Jun 28, 2020

Uh oh!

thomasjpfan Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Jul 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

glemaitre Jun 23, 2020 •

edited

Loading

thomasjpfan Jun 27, 2020 •

edited

Loading

claramatos commented Jun 27, 2020 •

edited

Loading

thomasjpfan Jun 27, 2020 •

edited

Loading