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] Allow plot_confusion_matrix to be called on predicted labels #15883

Closed

Conversation

jhennrich
Copy link

Allow passing predicted labels y_pred to plot_confusion_matrix that will be used instead of estimator and X. In order to maintain backwards compatibility, y_pred is added as an optional keyword argument.
When both estimator and X aswell as y_pred are passed, a ValueError is raised since it is unclear which of both should be used.

For the discussion see #15880

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @jhennrich

Don't worry about CI failing for now, it's unrelated.

please add a few tests. We would also need a short sentence in the user guide (doc/modules/model_evaluation.rst) to document this feature

sklearn/metrics/_plot/confusion_matrix.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/confusion_matrix.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/confusion_matrix.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/confusion_matrix.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/confusion_matrix.py Outdated Show resolved Hide resolved
sklearn/metrics/_plot/confusion_matrix.py Show resolved Hide resolved
@qinhanmin2014
Copy link
Member

thanks, please apply similar changes to plot_roc_curve and plot_precision_recall_curve

@NicolasHug
Copy link
Member

Maybe we can make changes to the other functions in another PR

@jhennrich
Copy link
Author

@NicolasHug: I fixed everything you suggested and added tests.

@marctorsoc
Copy link
Contributor

marctorsoc commented Jan 23, 2020

Hi, what happened with this? it seems ready to be merged. What are the plans for ROC curve and others? I feel quite inefficient having to predict again if I want to plot the ROC curve

On a similar matter, what do you think in general about having just one function with a boolean to plot? wouldn't that simplify a lot the API?

confusion_matrix(y_true, y_pred)   ----->
                                             confusion_matrix(...., plot={True, False})
plot_confusion_metric(estimator, X, y_true) ---> 

I think this would be nice because maybe you want to plot the CM, ROC, etc, and also have it. For example, you wanna plot the ROC curve and choose the threshold

@thomasjpfan
Copy link
Member

@marctorrellas There is a discussion on #15880 going through if we want inflate the API of this function.

In summary, having two interfaces in one function feels like it will lead to an inflated API. The way to do this with already computed predictions would be to:

from sklearn.metrics import ConfusionMatrixDisplay

y_pred = est.predict(...)
confusion_matrix = confusion_matrix(y_true, y_pred)
display_labels = [...]

disp = ConfusionMatrixDisplay(confusion_matrix=confusion_matrix,
                              display_labels=display_labels).plot(...)

@marctorsoc
Copy link
Contributor

@marctorrellas There is a discussion on #15880 going through if we want inflate the API of this function.

In summary, having two interfaces in one function feels like it will lead to an inflated API. The way to do this with already computed predictions would be to:

from sklearn.metrics import ConfusionMatrixDisplay

y_pred = est.predict(...)
confusion_matrix = confusion_matrix(y_true, y_pred)
display_labels = [...]

disp = ConfusionMatrixDisplay(confusion_matrix=confusion_matrix,
                              display_labels=display_labels).plot(...)

thanks so much for your answer! didn't realise about those *****Display 👍

Base automatically changed from master to main January 22, 2021 10:51
@thomasjpfan
Copy link
Member

Now that ConfusionMatrix.from_predictions is released, we can now plot the confusion matrix from the predictions directly. With that in mind, I am closing this PR. Thank you for working on this PR!

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

Successfully merging this pull request may close these issues.

None yet

5 participants