Skip to content
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

[MRG] ENH Adds plot_confusion matrix #15083

Merged
merged 32 commits into from Nov 14, 2019

Conversation

thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Sep 24, 2019

Reference Issues/PRs

Related to #7116

What does this implement/fix? Explain your changes.

Adds plotting function for the confusion matrix.

@amueller
Copy link
Member

@amueller amueller commented Sep 24, 2019

lint ;)

fmt = '.2f' if self.normalize else 'd'
thresh = cm.max() / 2.
for i, j in product(range(cm.shape[0]), range(cm.shape[1])):
color = "white" if cm[i, j] < thresh else "black"
Copy link
Member

@amueller amueller Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's weird as it doesn't depend on the colormap.
Here's how I usually do it:
https://github.com/amueller/mglearn/blob/master/mglearn/tools.py#L76

without depending on the colormap there's no way this works, right? because someone could use greys and greys_r and they clearly need the opposite colors.

Copy link
Member

@amueller amueller Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be pcolormesh not pcolor, though.

Copy link
Member

@amueller amueller Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also: shouldn't this go in a separate helper function? It's probably not the only time we want to show a heatmap (grid search will need this as well). The main question then is if that will be public or not :-/

Copy link
Member Author

@thomasjpfan thomasjpfan Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@amueller amueller Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.
Can you maybe add a test? Like calling ConfusionMatrixDisplay with np.eye(2) and plt.cm.greys and check that the text colors are black white black white and with plt.cm.greys_r and check that the text colors are white black white black?

titles_options = [("Confusion matrix, without normalization", False),
("Normalized confusion matrix", True)]
for title, normalize in titles_options:
fig, ax = plt.subplots()
Copy link
Member

@amueller amueller Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? There's no reason to pass ax, right?
For setting the title you could just do plt.gca().set_title(title).
Or do you knot like using the state like that?

Copy link
Member Author

@thomasjpfan thomasjpfan Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with not having to define ax and passing it in and using the axes stored in the Display object.

Copy link
Member

@amueller amueller Sep 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, even better.

@amueller
Copy link
Member

@amueller amueller commented Sep 25, 2019

how about adding it in all the examples that use confusion_matrix:
https://scikit-learn.org/dev/modules/generated/sklearn.metrics.confusion_matrix.html#sklearn.metrics.confusion_matrix

select a subset of labels. If `None` is given, those that appear at
least once in `y_true` or `y_pred` are used in sorted order.
target_names : array-like of shape (n_classes,), default=None
Copy link
Member

@jnothman jnothman Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't call this target names. That implies multiple targets. Rather, display_labels will be sufficient?

Copy link
Member Author

@thomasjpfan thomasjpfan Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, do you think classes or class_names would be better?

Includes values in confusion matrix.
normalize : bool, default=False
Normalizes confusion matrix.
Copy link
Member

@jnothman jnothman Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user might want to normalise over either axis, or altogether.

Copy link
Member Author

@thomasjpfan thomasjpfan Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four options? I guess we can do 'row', 'column', 'all', None?

Copy link
Member

@jnothman jnothman Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay to not provide this flexibility, too. Another way to specify it is "all", "recall", "precision", None.

Copy link
Contributor

@glemaitre glemaitre Oct 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use "truth" and "predicted" instead of "recall" and "precision"?

Copy link
Member Author

@thomasjpfan thomasjpfan Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR to use 'truth' and 'predicted'. Almost feels like this should be in confusion_matrix itself.

@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Oct 23, 2019

Updated with using display_labels and using the dtype of confusion matrix to infer if the matrix is normalized.

@thomasjpfan thomasjpfan added this to the 0.22 milestone Oct 25, 2019
@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Oct 25, 2019

Copy link
Member

@NicolasHug NicolasHug left a comment

cmap='viridis', ax=None):
"""Plot Confusion Matrix.
Read more in the :ref:`User Guide <visualizations>`.
Copy link
Member

@NicolasHug NicolasHug Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrinjalali adrinjalali added this to In progress in Meeting Issues via automation Oct 29, 2019
@jnothman
Copy link
Member

@jnothman jnothman commented Nov 6, 2019

Copy link
Contributor

@glemaitre glemaitre left a comment

With the latest changes, LGTM

Rotation of xtick labels.
values_format : str, default=None
Format specification for values in confusion matrix. If None,
Copy link
Contributor

@glemaitre glemaitre Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jus this nitpick

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Nov 7, 2019

@glemaitre yes our classifiers work with list of strings, but out simple example using load_iris returns the integer encoding and not the strings. A user using load_iris will need to pass in the display_labels=iris.target_names to get the expected labeling.

So it seems that we need them in case we want to overwrite it. So we can keep it has it is until by default we don't need to specify it.

Copy link
Member

@NicolasHug NicolasHug left a comment

Thanks @thomasjpfan , mostly looks good.

I'm slightly concerned about testing time and coupling though

Includes values in confusion matrix.
normalize : {'true', 'pred', 'all'}, default=None
Normalizes confusion matrix over the true, predicited conditions or
Copy link
Member

@NicolasHug NicolasHug Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion

Suggested change
Normalizes confusion matrix over the true, predicited conditions or
Normalizes confusion matrix over the true (rows), predicited conditions (columns) or

labels=labels)

if normalize == 'true':
cm = cm.astype('float') / cm.sum(axis=1, keepdims=True)
Copy link
Member

@NicolasHug NicolasHug Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not convert to float (see other msg about high coupling)


cm = self.confusion_matrix
n_classes = cm.shape[0]
normalized = np.issubdtype(cm.dtype, np.float_)
Copy link
Member

@NicolasHug NicolasHug Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic involves a strong coupling between

confusion_matrix -> plot_confusion_matrix -> ConfusionMatrixDisplay

and might cause silent bugs in the future.

I would rather pass a is_normalized parameter (or remove, see below)

if include_values:
self.text_ = np.empty_like(cm, dtype=object)
if values_format is None:
values_format = '.2f' if normalized else 'd'
Copy link
Member

@NicolasHug NicolasHug Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the .2g option is what we need, and you wouldn't have to use the normalized variable anymore:

In [15]: "{:.2g} -- {:.2g} -- {:.2g}".format(2, 2.0000, 2.23425)                                                                        
Out[15]: '2 -- 2 -- 2.2'

sklearn/metrics/_plot/confusion_matrix.py Show resolved Hide resolved
sklearn/metrics/_plot/confusion_matrix.py Show resolved Hide resolved
@pytest.mark.parametrize("normalize", ['true', 'pred', 'all', None])
@pytest.mark.parametrize("with_sample_weight", [True, False])
@pytest.mark.parametrize("with_labels", [True, False])
@pytest.mark.parametrize("cmap", ['viridis', 'plasma'])
@pytest.mark.parametrize("with_custom_axes", [True, False])
@pytest.mark.parametrize("with_display_labels", [True, False])
@pytest.mark.parametrize("include_values", [True, False])
Copy link
Member

@NicolasHug NicolasHug Nov 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need each of these combinations to be tested independently?

It seems to me that most of the checks in this test could be independent tests functions. Parametrization is nice but seems way overkill here.

This will test 256 instances, and it take about 10s on my machine which is not negligible considering small increment in testing time really add up over time.

@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Nov 7, 2019

To be consistent with the plot_roc_curve, how do you feel about names or display_names instead of names?

@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Nov 7, 2019

Ah display_labels is okay, since this is this a different context. Updated PR to reduce the number of tests and to address comments.

Interpretability / Plotting / Interactive dev automation moved this from Review in progress to Reviewer approved Nov 8, 2019
Copy link
Member

@NicolasHug NicolasHug left a comment

last nits

assert disp.ax_ == ax

if normalize == 'true':
cm = cm.astype('float') / cm.sum(axis=1, keepdims=True)
Copy link
Member

@NicolasHug NicolasHug Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you dont need the conversion anymore right?

@pytest.mark.parametrize("normalize", ['true', 'pred', 'all', None])
@pytest.mark.parametrize("with_labels", [True, False])
@pytest.mark.parametrize("with_display_labels", [True, False])
@pytest.mark.parametrize("include_values", [True, False])
Copy link
Member

@NicolasHug NicolasHug Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason I'm not a fan of this is that such parametrization suggests that all these 4 parameters are intertwined and are dependent one to another, but in reality this isn't the case

I think we could still remove some parametrizations, but that's fine

create a :class:`ConfusionMatrixDisplay`. All parameters are stored as
attributes.
Read more in the :ref:`User Guide <confusion_matrix>`.
Copy link
Member

@NicolasHug NicolasHug Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this link to the visualization UG?

Meeting Issues automation moved this from Review in progress to Reviewer approved Nov 8, 2019
include_values : bool, default=True
Includes values in confusion matrix.
normalize : {'true', 'pred', 'all'}, default=None
Copy link
Member

@qinhanmin2014 qinhanmin2014 Nov 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to support normalize here, perhaps we should also support it in confusion_matrix (See #14478).
And I can't understand why we need normalize="all".

Copy link
Contributor

@glemaitre glemaitre Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good remark. normalize='all' will normalize by the total support.

Copy link
Contributor

@glemaitre glemaitre Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I would suggest to add it to another PR.

@glemaitre glemaitre self-assigned this Nov 14, 2019
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Nov 14, 2019

I made a push to solve the conflicts

@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Nov 14, 2019

and I added a similar test to the other plotting for pipeline.
@qinhanmin2014 feel free to merge when it is green

@glemaitre glemaitre merged commit e650a20 into scikit-learn:master Nov 14, 2019
21 checks passed
Interpretability / Plotting / Interactive dev automation moved this from Reviewer approved to Done Nov 14, 2019
Meeting Issues automation moved this from Reviewer approved to Done Nov 14, 2019
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Nov 14, 2019

OK merging this one. I will open a new PR to address the problem raised by @qinhanmin2014 in #15083 (comment)

adrinjalali pushed a commit to adrinjalali/scikit-learn that referenced this issue Nov 18, 2019
adrinjalali pushed a commit to adrinjalali/scikit-learn that referenced this issue Nov 18, 2019
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this issue Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants