DOC Clarify what can be passed to the plotting utilities#16893
DOC Clarify what can be passed to the plotting utilities#16893thomasjpfan merged 5 commits intoscikit-learn:masterfrom justmarkham:patch-1
Conversation
thomasjpfan
left a comment
There was a problem hiding this comment.
Making the docstring clearer here is reasonable. I think I am in favor of:
Fitted classifier, or a fitted pipeline in which the last estimator is a classifier.
Since this is closest to the fit method.
|
@thomasjpfan Thanks for the feedback! I went ahead and tweaked the wording, and applied the same wording to If this PR is approved, would you mind advising me on the next steps? I assume there is some squashing of commits that takes place, but I'm not familiar with how to do that and any conventions that I would need to keep in mind. Thanks! |
|
@justmarkham Pushing commits to this PR, as you are doing, is the preferred workflow. We prefer this because it allows the discussion's history to have the context of the previous commits. When we merge with master, we are able to squash and merge the commits. |
NicolasHug
left a comment
There was a problem hiding this comment.
Minor suggestion but LGTM, thanks @justmarkham
|
@thomasjpfan @NicolasHug Thanks! I've incorporated your feedback, and I'm all done with these changes unless there is additional feedback. |
thomasjpfan
left a comment
There was a problem hiding this comment.
Thank you @justmarkham !
LGTM
Reference Issues/PRs
None
What does this implement/fix? Explain your changes.
The documentation currently states that a "trained classifier" can be passed to
plot_confusion_matrix. I think it would be useful to note explicitly that a pipeline ending in a classifier can also be passed toplot_confusion_matrix.Any other comments?
Here are some options for the wording:
If this PR is accepted, I'd be happy to also edit the docs for
plot_precision_recall_curveandplot_roc_curvein a similar way (after I confirm that one can pass a pipeline to both of those as well).