ENH/DEP add class method and deprecate plot function for confusion matrix #18543
Conversation
…ot_confusion_matrix
|
@glemaitre While working on this PR, do you think that |
To be honest, I prefer the long version especially for |
|
@thomasjpfan I think this is ready for being reviewed. Note that here, I did not change the visualization guideline. I think it would be best to change it at the same time as the ROC AUC display. Here I am introducing a new utility to make docstring substitution as in |
|
@NicolasHug @thomasjpfan @ogrisel @amueller @jnothman We can iterate on this PR such that it gets through and it will make the other PR easier to review then. |
Thanks @glemaitre , a few minor comments but this looks good! thanks for working on this
To be honest, I prefer the long version
I agree
| @@ -0,0 +1,328 @@ | |||
| from numpy.testing import ( | |||
There was a problem hiding this comment.
This is the same as the previous test file, with some minor adjustements, right?
Should we also test that both class methods yield the same plots? Maybe checking the confusion_matrix attribute is enough
There was a problem hiding this comment.
Yes, it is the same file but I used a fixture to test both constructor. Since some of the tests already compare to the confusion_matrix function output, I don't think that we need to check further.
|
@NicolasHug @thomasjpfan I removed the super meta-fixture :) |
|
Then how does it work with indentations? Does sphinx not get confused if
the indentations are not the same for all the parameters in the following
lines in the docstring?
This will be one of the limitations. If you have a function or a method in
the class with different indentations, then you will have an issue to
inject because you would need different docstrings to be injected.
So since the injection seems quite controversial, I will remove it from
this PR and go with replicating the docstring and go forward with this PR.
We can give another try for the injection in another PR/issue :)
…On Sat, 17 Oct 2020 at 10:47, Adrin Jalali ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Assuming we go down the path of docstring injection, I think it'd make
more sense to have them per parameter rather than a big chunk alltogher.
I'd separate the injections per parameter, and put them somewhere to
collect all common docstrings, like common_docstrings.py.
And I think we don't need to inject the Returns section, that can be just
stay in the docstring itself.
Then how does it work with indentations? Does scphinx not get confused if
the indentations are not the same for all the parameters in the following
lines in the docstring?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18543 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABY32PZ4MN3PGQPTG5WR2ETSLFK25ANCNFSM4SFXXS4A>
.
--
Guillaume Lemaitre
Scikit-learn @ Inria Foundation
https://glemaitre.github.io/
|
This reverts commit b64f2d1.
|
@adrinjalali Do you think that we can go forward? |
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
|
ping @adrinjalali Would it be fine to go ahead. |
Thanks @glemaitre . I haven't checked the actual plots, otherwise looks pretty good :)
Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>
|
Merging since the CIs are green and there is 2 approvals |
Reference Issues/PRs
Addess some of #15880
What does this implement/fix? Explain your changes.
Introduce 2 class methods for
ConfusionMatrixDisplayand deprecate theplot_function.This is the first of many successive PR. It is introducing the
Substitutionclass to inject recurrent docstring and avoid duplicating code.Any other comments?
The text was updated successfully, but these errors were encountered: