ENH Add parameter in colorbar in plot_confusion_matrix #17192

avigupta2612 · 2020-05-12T15:23:52Z

Reference Issues/PRs

Added feature to make colorbar optional as mentioned in #16519

What does this implement/fix? Explain your changes.

Added an argument colorbar in plot_confusion_matrix function and Set the colorbar to True by Default
Made same changes to the Confusion matrix display class's plot function and changed
'
fig.colorbar(self.im_, ax=ax)
'
to
'
if self.colorbar:
fig.colorbar(self.im_, ax=ax)
'

Any other comments?

avigupta2612 · 2020-05-12T17:35:23Z

Can anyone tell what what went wrong in this and how can I correct this?

cmarmo · 2020-05-12T17:46:47Z

Hi @avigupta2612, your code have a linting issue.
scikit-learn use lint flake8 directives: in order to check your code locally you can run

$ flake8 <your_modified_file>.py

Hope this can help.

avigupta2612 · 2020-05-12T18:57:14Z

So this is my first PR. Plot_confusion_matrix tests are failing. So, do I need to edit the tests? If so, how can I do it?

cmarmo · 2020-05-12T19:16:57Z

Tests related to each module of scikit-learn are in sklearn/<name_of_the_module>/tests/ directory. The build logs (e.g. this one) tell you which test is failing and the specific error.
In particular, you are in the process of adding an argument to plot_confusion_matrix, you should then check where in the tests this argument needs to be explicitly added.

cmarmo

Thanks @avigupta2612! Also, please add a new entry in the "what's new" file for version 0.24, describing your addition.

cmarmo · 2020-05-13T15:00:58Z

sklearn/metrics/_plot/confusion_matrix.py

@@ -70,6 +71,7 @@ def plot(self, *, include_values=True, cmap='viridis',
            Axes object to plot on. If `None`, a new figure and axes is
            created.

+        colorbar : matplotlib colorbar, default=True
        Returns


If you check the rendering of the documentation you see that you need a blank line before the "Returns" paragraph in order to keep the correct indentation.

cmarmo · 2020-05-13T15:16:14Z

sklearn/metrics/_plot/confusion_matrix.py

@@ -70,6 +71,7 @@ def plot(self, *, include_values=True, cmap='viridis',
            Axes object to plot on. If `None`, a new figure and axes is
            created.

+        colorbar : matplotlib colorbar, default=True


You are adding a new parameter, a useful resource is the scikit-learn contributing documentation, in order to standardize the parameter description.
Also, could you please add a versionadded directive? You can check #15426 for examples about how this should be addressed.

…into colorbar

avigupta2612 · 2020-05-13T17:26:32Z

Thanks @cmarmo for reviewing. I have made some changes as you suggested, please have a look at them.

cmarmo · 2020-05-13T18:54:55Z

Last comments on my side.
I think that the colorbar parameter should be described as a boolean, this is the way you are using it in the code.
I'm not sure about the label to attach to this feature in the "what's new".

Let's wait for other reviewers.
And, please, be patient: the release is just out and people are really busy fixing all comments about the upgrade.
Thanks for your work!

sklearn/metrics/_plot/confusion_matrix.py

amueller

Looks good. Maybe check that by default the colorbar is not None?

sklearn/metrics/_plot/confusion_matrix.py

doc/whats_new/v0.24.rst

Co-authored-by: Venkatachalam N <venky.yuvy@gmail.com>

glemaitre

I think that we can improve the test mainly just to cover all possible use cases.

sklearn/metrics/_plot/confusion_matrix.py

glemaitre · 2020-05-27T14:00:42Z

sklearn/metrics/_plot/tests/test_plot_confusion_matrix.py

@@ -173,6 +173,9 @@ def test_confusion_matrix_display(pyplot, data, fitted_clf, y_pred, n_classes):
    image_data = disp.im_.get_array().data
    assert_allclose(image_data, cm)

+    disp.plot(colorbar=False)
+    assert disp.im_.colorbar is None


I think that we should add a separate test to check the effect of the parameter.
Indeed, it is appearing in disp.plot and plot_confusion_matrix so we should check that the behaviour is correct.

@pytest.mark.parametrize("colorbar, [True, False]) def test_plot_confusion_matrix_colorbar(colorbar): disp = plot_confusion_matrix(..., colorbar=colorbar) assert disp.im_.colorbar ... # attempt a plot with the opposit effect disp.plot(colorbar=not colorbar) assert disp.im_.colorbar ...

This still needs to be addressed.

cmarmo · 2020-06-21T12:11:47Z

Hi @avigupta2612 , you already have one approval. Do you have some time to fix conflicts and apply last comments? Thanks for your work!

avigupta2612 · 2020-06-25T20:55:57Z

@cmarmo I made the changes as per this 9fcc4d9 commit.

cmarmo · 2020-06-25T21:27:35Z

@glemaitre or perhaps @thomasjpfan, all green here and one approval already. Do you mind having a look? Thanks!

thomasjpfan

Thank you for the PR @avigupta2612 !

thomasjpfan · 2020-06-26T00:16:45Z

sklearn/metrics/_plot/tests/test_plot_confusion_matrix.py

@@ -173,6 +173,9 @@ def test_confusion_matrix_display(pyplot, data, fitted_clf, y_pred, n_classes):
    image_data = disp.im_.get_array().data
    assert_allclose(image_data, cm)

+    disp.plot(colorbar=False)
+    assert disp.im_.colorbar is None


This still needs to be addressed.

avigupta2612 · 2020-06-26T11:48:11Z

I created a separate test for colorbar as suggestes by @glemaitre. I am running lint test and it is working fine but when I push the changes the tests are faling can you help here @cmarmo

cmarmo · 2020-06-26T11:54:14Z

Some conflicts with upstream raised up, in the meanwhile. Merging with upstream (and solving conflicts), will solve your issues.

cmarmo · 2020-06-26T12:19:54Z

doc/whats_new/v0.24.rst

+<<<<<<< HEAD
+=======


Those two lines should be removed: they are the relic of the automatic git diff with upstream

cmarmo · 2020-06-26T12:20:20Z

doc/whats_new/v0.24.rst

 :mod:`sklearn.datasets`
 .......................
 - |Feature| :func:`datasets.fetch_openml` now validates md5checksum of arff
  files downloaded or cached to ensure data integrity.
  :pr:`14800` by :user:`Shashank Singh <shashanksingh28>` and `Joel Nothman`_.

+>>>>>>> upstream/master


Same here: to be removed.

cmarmo · 2020-06-26T12:23:03Z

doc/whats_new/v0.24.rst

 :mod:`sklearn.datasets`
 .......................
 - |Feature| :func:`datasets.fetch_openml` now validates md5checksum of arff
  files downloaded or cached to ensure data integrity.
  :pr:`14800` by :user:`Shashank Singh <shashanksingh28>` and `Joel Nothman`_.



Indeed the entire block should be removed: it is already present on the top.
Modules are in alphabetical order in the what's new file.

cmarmo · 2020-06-26T13:02:24Z

@thomasjpfan all is green here but I did a mistake: the current what's new page contains two entries for the sklearn.datasets module (see here). I asked @avigupta2612 to remove the second one but it wasn't a duplicate. Can we merge this PR and I will propose a fix for my mistake? I don't have edit rights and I think that a lot of syncs have already be done in this PR. Thanks for your understanding.

avigupta2612 · 2020-06-26T13:19:40Z

Thanks @cmarmo for your help. The whats_new file I updated (here) and the main page here both contain same entries for sklearn.datasets module. I guess the entry I removed in the previous commit was a third entry in sklearn.datasets

cmarmo · 2020-06-26T13:22:57Z

Thanks @cmarmo for your help. The whats_new file I updated (here) and the main page here both contain same entries for sklearn.datasets module. I guess the entry I removed in the previous commit was a third entry in sklearn.datasets

Right, if you still have some time do you mind re-adding it at the beginning of the file? Then I hope that you don't need to re-synchronize again before approval. Thanks for your patience.

avigupta2612 · 2020-06-26T13:39:12Z

@cmarmo I guess this third entry for sklearn.datasets has been updated incorrectly in the main master branch here. Should I change it accoring to the master branch or should I include it in the sklearn.datasets section?
Or If I can give you the edit rights to you tell me how to do it so that you can change it accordingly.

cmarmo · 2020-06-26T13:49:26Z

@cmarmo I guess this third entry for sklearn.datasets has been updated incorrectly in the main master branch here. Should I change it accoring to the master branch or should I include it in the sklearn.datasets section?

I think the best option is keep it where it is in master and fix this in another pull request. Thanks.

Or If I can give you the edit rights to you tell me how to do it so that you can change it accordingly.

You are very reactive and you know how to fix the issue, no need for that. :)

glemaitre

2 changes and we are good to go.

glemaitre · 2020-06-26T18:52:23Z

doc/whats_new/v0.24.rst

@@ -230,12 +234,6 @@ Changelog
  :meth:`tree.DecisionTreeRegressor.fit`, and has not effect.
  :pr:`17614` by :user:`Juan Carlos Alfaro Jiménez <alfaro96>`.

-:mod:`sklearn.datasets`


You should reintroduce these lines (233-238) at the exact same position.

I have included entry for my change at lines (145-148). So shouldn't I include skleanrn.datasets entry at lines 237-242?

These lines are red in the diff because you removed them (probably not on purpose but when fixing a conflict). Thus, you should reinsert such that we don't see any diff on GitHub (meaning that you did not edit the lines apart of your additional entry).

glemaitre · 2020-06-26T18:54:18Z

sklearn/metrics/_plot/tests/test_plot_confusion_matrix.py

+    disp = plot_confusion_matrix(fitted_clf, X, y, colorbar=True)
+    assert disp.im_.colorbar is not None
+
+    # attempt a plot with the opposit effect
+    disp.plot(colorbar=False)
+    assert disp.im_.colorbar is None


Suggested change

disp = plot_confusion_matrix(fitted_clf, X, y, colorbar=True)

assert disp.im_.colorbar is not None

# attempt a plot with the opposit effect

disp.plot(colorbar=False)

assert disp.im_.colorbar is None

def _check_colorbar(disp, has_colorbar):

if has_colorbar:

assert disp.im_.colorbar is not None

assert disp.im_.colorbar.__class__.__name__ == "Colorbar"

else:

assert disp.im_.colorbar is None

disp = plot_confusion_matrix(fitted_clf, X, y, colorbar=colorbar)

_check_colorbar(disp, colorbar)

# attempt a plot with the opposite effect of colorbar

disp.plot(colorbar=not colorbar)

_check_colorbar(disp, not colorbar)

We need to use the colorbar created by the parametrization.

avigupta2612 · 2020-06-26T20:44:29Z

@glemaitre I have made all the changes as you suggested thanks for your help.

glemaitre · 2020-06-26T20:58:29Z

@avigupta2612 Thanks for the work

…17192) Co-authored-by: Venkatachalam N <venky.yuvy@gmail.com>

optional colorbar

c24b021

github-actions bot added the module:metrics label May 12, 2020

lint edit

6b0008c

test edit

5951667

cmarmo reviewed May 13, 2020

View reviewed changes

avigupta2612 added 3 commits May 13, 2020 21:16

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

54ffeab

…into colorbar

doc

b7b895c

versionadded

386b9cd

amueller reviewed May 13, 2020

View reviewed changes

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

amueller reviewed May 13, 2020

View reviewed changes

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

bool

b18e9d3

amueller approved these changes May 14, 2020

View reviewed changes

avigupta2612 commented May 14, 2020

View reviewed changes

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

venkyyuvy reviewed May 16, 2020

View reviewed changes

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

Update doc/whats_new/v0.24.rst

741b3b0

Co-authored-by: Venkatachalam N <venky.yuvy@gmail.com>

cmarmo added the Waiting for Reviewer label May 27, 2020

glemaitre changed the title ~~Added argument in plot_confusion_matrix to make colorbar optional~~ ENH Add parameter in colorbar in plot_confusion_matrix May 27, 2020

glemaitre removed the Waiting for Reviewer label May 27, 2020

glemaitre reviewed May 27, 2020

View reviewed changes

glemaitre added this to REVIEWED AND WAITING FOR CHANGES in Guillaume's pet May 27, 2020

cmarmo added Waiting for Reviewer and removed Waiting for Reviewer labels Jun 21, 2020

cmarmo mentioned this pull request Jun 21, 2020

Make colorbar in plot_confusion_matrix optional #16519

Closed

Update confusion_matrix.py

172e5c2

thomasjpfan reviewed Jun 26, 2020

View reviewed changes

cmarmo removed the Waiting for Reviewer label Jun 26, 2020

test

af52446

avigupta2612 force-pushed the colorbar branch from 0c296ab to af52446 Compare June 26, 2020 11:44

conf

b26bd70

cmarmo reviewed Jun 26, 2020

View reviewed changes

new

2fd8603

Merge remote-tracking branch 'upstream/master' into colorbar

2364c4b

glemaitre self-requested a review June 26, 2020 18:22

glemaitre reviewed Jun 26, 2020

View reviewed changes

avigupta2612 added 2 commits June 27, 2020 01:45

Merge remote-tracking branch 'upstream/master' into colorbar

4274145

test

445e882

glemaitre merged commit 8dc143e into scikit-learn:master Jun 26, 2020

glemaitre moved this from REVIEWED AND WAITING FOR CHANGES to MERGED in Guillaume's pet Jul 7, 2020

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

ENH Add parameter in colorbar in plot_confusion_matrix (scikit-learn#…

37ddb61

…17192) Co-authored-by: Venkatachalam N <venky.yuvy@gmail.com>

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

ENH Add parameter in colorbar in plot_confusion_matrix (scikit-learn#…

eab3abe

…17192) Co-authored-by: Venkatachalam N <venky.yuvy@gmail.com>

ENH Add parameter in colorbar in plot_confusion_matrix #17192

ENH Add parameter in colorbar in plot_confusion_matrix #17192

avigupta2612 commented May 12, 2020

avigupta2612 commented May 12, 2020

cmarmo commented May 12, 2020

avigupta2612 commented May 12, 2020

cmarmo commented May 12, 2020

cmarmo left a comment

cmarmo May 13, 2020

cmarmo May 13, 2020

avigupta2612 commented May 13, 2020

cmarmo commented May 13, 2020

amueller left a comment

glemaitre left a comment

glemaitre May 27, 2020

thomasjpfan Jun 26, 2020

cmarmo commented Jun 21, 2020

avigupta2612 commented Jun 25, 2020

cmarmo commented Jun 25, 2020

thomasjpfan left a comment

thomasjpfan Jun 26, 2020

avigupta2612 commented Jun 26, 2020

cmarmo commented Jun 26, 2020

cmarmo Jun 26, 2020

cmarmo Jun 26, 2020

cmarmo Jun 26, 2020

cmarmo commented Jun 26, 2020 •

edited

avigupta2612 commented Jun 26, 2020

cmarmo commented Jun 26, 2020 •

edited

avigupta2612 commented Jun 26, 2020 •

edited

cmarmo commented Jun 26, 2020 •

edited

glemaitre left a comment

glemaitre Jun 26, 2020

avigupta2612 Jun 26, 2020

glemaitre Jun 26, 2020

glemaitre Jun 26, 2020

glemaitre Jun 26, 2020

avigupta2612 commented Jun 26, 2020

glemaitre commented Jun 26, 2020

-    disp = plot_confusion_matrix(fitted_clf, X, y, colorbar=True)
-    assert disp.im_.colorbar is not None
-    # attempt a plot with the opposit effect
-    disp.plot(colorbar=False)
-    assert disp.im_.colorbar is None
+    def _check_colorbar(disp, has_colorbar):
+        if has_colorbar:
+            assert disp.im_.colorbar is not None
+            assert disp.im_.colorbar.__class__.__name__ == "Colorbar"
+        else:
+            assert disp.im_.colorbar is None
+    disp = plot_confusion_matrix(fitted_clf, X, y, colorbar=colorbar)
+    _check_colorbar(disp, colorbar)
+    # attempt a plot with the opposite effect of colorbar
+    disp.plot(colorbar=not colorbar)
+    _check_colorbar(disp, not colorbar)

		<<<<<<< HEAD
		=======

ENH Add parameter in colorbar in plot_confusion_matrix #17192

ENH Add parameter in colorbar in plot_confusion_matrix #17192

Conversation

avigupta2612 commented May 12, 2020

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

avigupta2612 commented May 12, 2020

cmarmo commented May 12, 2020

avigupta2612 commented May 12, 2020

cmarmo commented May 12, 2020

cmarmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avigupta2612 commented May 13, 2020

cmarmo commented May 13, 2020

amueller left a comment

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmarmo commented Jun 21, 2020

avigupta2612 commented Jun 25, 2020

cmarmo commented Jun 25, 2020

thomasjpfan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avigupta2612 commented Jun 26, 2020

cmarmo commented Jun 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmarmo commented Jun 26, 2020 • edited

avigupta2612 commented Jun 26, 2020

cmarmo commented Jun 26, 2020 • edited

avigupta2612 commented Jun 26, 2020 • edited

cmarmo commented Jun 26, 2020 • edited

glemaitre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avigupta2612 commented Jun 26, 2020

glemaitre commented Jun 26, 2020

cmarmo commented Jun 26, 2020 •

edited

cmarmo commented Jun 26, 2020 •

edited

avigupta2612 commented Jun 26, 2020 •

edited

cmarmo commented Jun 26, 2020 •

edited