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 add normalize parameter to confusion_matrix #15625

Merged
merged 6 commits into from Nov 15, 2019

Conversation

glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Nov 14, 2019

Reference Issues/PRs

closes #14478

What does this implement/fix? Explain your changes.

Move the normalization normalize from plot_confusion_matrix to confusion_matrix.
I added a couple of test and handle division by zeros without raising warning.

Any other comments?

@glemaitre
Copy link
Contributor Author

@glemaitre glemaitre commented Nov 14, 2019

@glemaitre glemaitre changed the title ENH add normalize parameter to confusion_matrix [MRG] ENH add normalize parameter to confusion_matrix Nov 14, 2019
@thomasjpfan thomasjpfan self-requested a review Nov 14, 2019
@glemaitre glemaitre added this to the 0.22 milestone Nov 14, 2019
Copy link
Member

@NicolasHug NicolasHug left a comment

Looks good. I agree it makes more sense to have it in confusion_matrix rather than in the plot utility

in ``y_true`` or ``y_pred`` are used in sorted order.
sample_weight : array-like of shape (n_samples,), default=None
Sample weights.
normalize : {'true', 'pred', 'all'}, default=None
Normalizes confusion matrix over the true (rows), predicted (columns)
conditions or all the population. If None, confusion matrix will not be
Copy link
Member

@NicolasHug NicolasHug Nov 14, 2019

Choose a reason for hiding this comment

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

what is "conditions"?

Copy link
Contributor Author

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

True conditions, predicted conditions. I think that it is one way to call true values and predicted values.

@@ -155,7 +155,7 @@ def plot_confusion_matrix(estimator, X, y_true, sample_weight=None,
Includes values in confusion matrix.
normalize : {'true', 'pred', 'all'}, default=None
Normalizes confusion matrix over the true (rows), predicited (columns)
Normalizes confusion matrix over the true (rows), predicted (columns)
conditions or all the population. If None, confusion matrix will not be
Copy link
Member

@NicolasHug NicolasHug Nov 14, 2019

Choose a reason for hiding this comment

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

same here lol

Copy link
Contributor Author

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

I copied it from here :). Seeking a bit more, this is defined in this way on wikipedia: https://en.wikipedia.org/wiki/Confusion_matrix

sklearn/metrics/tests/test_classification.py Outdated Show resolved Hide resolved
doc/modules/model_evaluation.rst Outdated Show resolved Hide resolved
Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

I'm still wondering why we need to divide the entire matrix (though I won't oppose).

@qinhanmin2014
Copy link
Member

@qinhanmin2014 qinhanmin2014 commented Nov 15, 2019

Seems that @jnothman don't like it? #14478 (comment)

@jnothman
Copy link
Member

@jnothman jnothman commented Nov 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants