ENH/DEP add class method and deprecate plot function for confusion matrix by glemaitre · Pull Request #18543 · scikit-learn/scikit-learn

glemaitre · 2020-10-06T09:06:35Z

Reference Issues/PRs

Addess some of #15880

What does this implement/fix? Explain your changes.

Introduce 2 class methods for ConfusionMatrixDisplay and deprecate the plot_ function.

This is the first of many successive PR. It is introducing the Substitution class to inject recurrent docstring and avoid duplicating code.

Any other comments?

…ot_confusion_matrix

thomasjpfan · 2020-10-06T16:38:45Z

@glemaitre While working on this PR, do you think that from_est and from_preds would be better because it it shorter? The whole *Display is already pretty verbose.

glemaitre · 2020-10-06T16:59:26Z

@glemaitre While working on this PR, do you think that from_est and from_preds would be better because it it shorter? The whole *Display is already pretty verbose.

To be honest, I prefer the long version especially for from_estimator. I find it more explicit. One can always rename the *Display at the import with as I think.

glemaitre · 2020-10-06T17:19:17Z

@thomasjpfan I think this is ready for being reviewed. Note that here, I did not change the visualization guideline. I think it would be best to change it at the same time as the ROC AUC display.

Here I am introducing a new utility to make docstring substitution as in matplotlib to avoid rewriting a long list of parameters. I can still revert it if it is considered as not needed. Otherwise, we might use it in other places.

glemaitre · 2020-10-06T17:23:08Z

@NicolasHug @thomasjpfan @ogrisel @amueller @jnothman

We can iterate on this PR such that it gets through and it will make the other PR easier to review then.

NicolasHug

Thanks @glemaitre , a few minor comments but this looks good! thanks for working on this

To be honest, I prefer the long version

I agree

doc/whats_new/v0.24.rst

sklearn/metrics/_plot/confusion_matrix.py

sklearn/utils/tests/test_docstring.py

sklearn/utils/_docstring.py

NicolasHug · 2020-10-06T18:39:42Z

sklearn/metrics/_plot/tests/test_confusion_matrix_display.py

@@ -0,0 +1,328 @@
+from numpy.testing import (


This is the same as the previous test file, with some minor adjustements, right?

Should we also test that both class methods yield the same plots? Maybe checking the confusion_matrix attribute is enough

Yes, it is the same file but I used a fixture to test both constructor. Since some of the tests already compare to the confusion_matrix function output, I don't think that we need to check further.

sklearn/metrics/_plot/confusion_matrix.py

sklearn/utils/_docstring.py

NicolasHug

Thanks @glemaitre , some grumpy morning comments but LGTM anyway!

sklearn/metrics/_plot/confusion_matrix.py

sklearn/metrics/_plot/tests/test_confusion_matrix_display.py

glemaitre · 2020-10-07T16:38:50Z

@NicolasHug @thomasjpfan I removed the super meta-fixture :)
I agree it makes the test easier to understand and more explicit.

glemaitre · 2020-10-17T10:02:08Z

Then how does it work with indentations? Does sphinx not get confused if

the indentations are not the same for all the parameters in the following lines in the docstring? This will be one of the limitations. If you have a function or a method in the class with different indentations, then you will have an issue to inject because you would need different docstrings to be injected. So since the injection seems quite controversial, I will remove it from this PR and go with replicating the docstring and go forward with this PR. We can give another try for the injection in another PR/issue :)

…

On Sat, 17 Oct 2020 at 10:47, Adrin Jalali ***@***.***> wrote: ***@***.**** requested changes on this pull request. Assuming we go down the path of docstring injection, I think it'd make more sense to have them per parameter rather than a big chunk alltogher. I'd separate the injections per parameter, and put them somewhere to collect all common docstrings, like common_docstrings.py. And I think we don't need to inject the Returns section, that can be just stay in the docstring itself. Then how does it work with indentations? Does scphinx not get confused if the indentations are not the same for all the parameters in the following lines in the docstring? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18543 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABY32PZ4MN3PGQPTG5WR2ETSLFK25ANCNFSM4SFXXS4A> .

-- Guillaume Lemaitre Scikit-learn @ Inria Foundation https://glemaitre.github.io/

This reverts commit b64f2d1.

glemaitre · 2020-10-19T12:36:41Z

@adrinjalali Do you think that we can go forward?

adrinjalali

Thanks, I'm happy with the docstrings as they are now :)

…matrix

thomasjpfan

Minor comment on tests, otherwise LGTM

sklearn/metrics/_plot/confusion_matrix.py

sklearn/metrics/_plot/tests/test_confusion_matrix_display.py

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

doc/whats_new/v0.24.rst

…matrix

glemaitre · 2021-01-19T11:08:09Z

ping @adrinjalali Would it be fine to go ahead.

adrinjalali

Thanks @glemaitre . I haven't checked the actual plots, otherwise looks pretty good :)

sklearn/metrics/_plot/confusion_matrix.py

sklearn/metrics/_plot/tests/test_confusion_matrix_display.py

sklearn/metrics/_plot/tests/test_plot_confusion_matrix.py

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

…trix

glemaitre · 2021-01-22T18:58:05Z

Merging since the CIs are green and there is 2 approvals

ENH/DEP add class method to display confusion matrix and deprecate pl…

bff9c61

…ot_confusion_matrix

glemaitre marked this pull request as draft October 6, 2020 09:06

github-actions bot added the module:metrics label Oct 6, 2020

glemaitre added 10 commits October 6, 2020 12:41

MNT add a tool to inject docstring

131d175

add docstring in private methods

7a6f326

add deprecation filter in test

0d27d42

remove warning documentation

57eafe9

DOC add an entry in whats new

aa1d7c3

update examples

ecb11f0

TST add new files

99edffa

TST add test display custom labels

46270b2

TST add tests for plotting ConfusionMatrixDisplay

987fd8e

TST adding back right error message for plot_confusion_matrix

59ecd00

TST add test with different complex pipeline

9a1641d

glemaitre added 3 commits October 6, 2020 19:00

DOC fix docstring

e2b4431

TST add test for docstring substitution

1b5ce74

TST add really the file

c1f0790

glemaitre marked this pull request as ready for review October 6, 2020 17:16

NicolasHug reviewed Oct 6, 2020

View reviewed changes

glemaitre added 3 commits October 7, 2020 10:35

first round addressing review

0633035

improve injecter

9fa42d3

Fix example docstring

0ee9b99

NicolasHug approved these changes Oct 7, 2020

View reviewed changes

glemaitre added 2 commits October 7, 2020 18:07

remove fixture for more explicit tests

ee1ba71

Add see also

1753197

glemaitre added 2 commits October 17, 2020 12:05

Revert "remove the need of double braces"

648e510

This reverts commit b64f2d1.

remove string injection

d9d6274

glemaitre removed the Needs Decision Requires decision label Oct 17, 2020

adrinjalali reviewed Oct 19, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into is/15880_confusion_…

ca85e7c

…matrix

glemaitre added the Waiting for Reviewer label Oct 21, 2020

fix

a5622fc

thomasjpfan reviewed Oct 31, 2020

View reviewed changes

sklearn/metrics/_plot/confusion_matrix.py Outdated Show resolved Hide resolved

sklearn/metrics/_plot/tests/test_confusion_matrix_display.py Outdated Show resolved Hide resolved

glemaitre and others added 2 commits November 3, 2020 10:22

Apply suggestions from code review

78fc223

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

MNT use explicit constructor

3075bbb

jnothman reviewed Dec 29, 2020

View reviewed changes

doc/whats_new/v0.24.rst Outdated Show resolved Hide resolved

glemaitre added 3 commits January 5, 2021 09:37

Merge remote-tracking branch 'origin/master' into is/15880_confusion_…

9e08343

…matrix

MNT update deprecation version

9ccd1cc

Merge remote-tracking branch 'origin/master' into is/15880_confusion_…

9870fc5

…matrix

adrinjalali reviewed Jan 20, 2021

View reviewed changes

glemaitre and others added 2 commits January 20, 2021 11:16

Apply suggestions from code review

c7e52d6

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

address adrin comments

f1027bc

adrinjalali approved these changes Jan 22, 2021

View reviewed changes

Base automatically changed from master to main January 22, 2021 10:53

glemaitre added 2 commits January 22, 2021 19:07

Merge remote-tracking branch 'origin/main' into is/15880_confusion_ma…

2c06de3

…trix

reorder whats new

29e8f1d

glemaitre merged commit 8c6a045 into scikit-learn:main Jan 22, 2021

This was referenced Jan 26, 2021

DOC Remove plot_confusion_matrix in See Also #19277

Merged

ENH Add CalibrationDisplay plotting class #17443

Merged

glemaitre mentioned this pull request Apr 22, 2021

Release 0.24.2 #19954

Merged

12 tasks

glemaitre mentioned this pull request Jul 19, 2021

Suggestion: Remove prediction from plot_confusion_matrix and just pass predicted labels #15880

Closed

5 tasks

Uh oh!

Conversation

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

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

thomasjpfan commented Oct 6, 2020

Uh oh!

glemaitre commented Oct 6, 2020

Uh oh!

glemaitre commented Oct 6, 2020

Uh oh!

glemaitre commented Oct 6, 2020

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NicolasHug Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

glemaitre Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glemaitre commented Oct 7, 2020

Uh oh!

glemaitre commented Oct 17, 2020 via email

Uh oh!

glemaitre commented Oct 19, 2020

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glemaitre commented Jan 19, 2021

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glemaitre commented Jan 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

glemaitre commented Oct 6, 2020 •

edited

Loading