ENH kind accept a list of str in plot_partial_dependence #19438
Conversation
|
@thomasjpfan @NicolasHug This should make the job I think. |
|
Basically I see the new features of specifying a list as unrelated to the warning / error mechanism. In fact, I feel like this list syntax is the "natural" way of specifying |
This is one of the reason that I have 2 entries in the what's new. I can split the PR into 2. One to support the list as an enhancement and another one as a "bug" fix that introduce the warning. It will not make a huge change in the review process thought. |
|
I agree a single PR is fine, I was trying to explain my reasoning behind #19438 (comment) :) |
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
|
gentle ping to @ogrisel @thomasjpfan |
doc/whats_new/v1.1.rst
Outdated
| - |Enhancement| :class:`linear_model.RidgeClassifier` is now supporting | ||
| multilabel classification. |
There was a problem hiding this comment.
Unrelated whats new entries. Could be related to #21516 ?
There was a problem hiding this comment.
I moved them because they were not in the right section.
There was a problem hiding this comment.
Ah I see. Usually we do not do clean whats new clean ups in feature PRs?
There was a problem hiding this comment.
OK let me revert it and I will make a push the merging day in main.
| default_pd_lines_kws = { | ||
| "color": "tab:orange", | ||
| "linestyle": "--", | ||
| } |
There was a problem hiding this comment.
Instead of deleting ice_lines_kw["label"] here, I think we can set label here:
| default_pd_lines_kws = { | |
| "color": "tab:orange", | |
| "linestyle": "--", | |
| } | |
| default_pd_lines_kws = { | |
| "color": "tab:orange", | |
| "linestyle": "--", | |
| "label": "average" | |
| } |
There was a problem hiding this comment.
it is not enough because "label" is part of default_line_kws
There was a problem hiding this comment.
The problem comes if a user provides a "label" key when calling plot to overwrite the label. Then we need to delete the key in some cases.
There was a problem hiding this comment.
I forgot to finish the thought. I thought we could leave the label out of default_line_kws too, since it only applies to default_pd_lines_kws.
The problem comes if a user provides a "label" key when calling plot to overwrite the label. Then we need to delete the key in some cases.
I think I see what you mean. If line_kw has a "label", then we want to remove the label for ice plots?
Scrolling up I see this is the same logic we had before, so I am okay with keeping it as is.
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
doc/whats_new/v1.1.rst
Outdated
| - |Enhancement| :class:`linear_model.RidgeClassifier` is now supporting | ||
| multilabel classification. |
There was a problem hiding this comment.
Ah I see. Usually we do not do clean whats new clean ups in feature PRs?
| default_pd_lines_kws = { | ||
| "color": "tab:orange", | ||
| "linestyle": "--", | ||
| } |
There was a problem hiding this comment.
I forgot to finish the thought. I thought we could leave the label out of default_line_kws too, since it only applies to default_pd_lines_kws.
The problem comes if a user provides a "label" key when calling plot to overwrite the label. Then we need to delete the key in some cases.
I think I see what you mean. If line_kw has a "label", then we want to remove the label for ice plots?
Scrolling up I see this is the same logic we had before, so I am okay with keeping it as is.
|
So I am waiting for the CI and I will merge. I will make a subsequent PR to solve the misordering of the changelog |
…n#19438) Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
closes #19305
This PR proposes that:
kindsupports a list of str of the same length asfeaturesto specify different PD plots for each axis.kind="both"and a 2-way partial dependence is requested in feature, a warning is raised instead of not making any plot. The warning will stipulate to pass a list to silence it.