API Deprecates plot_partial_dependence #20959
Conversation
|
This is the remaining plotting function that does not have a class method. Placing on 1.0 |
|
I wanted to have:
before to touch this. I was feeling that it would be much less of a hassle than solving the conflict :) |
|
I am going to review it in the morning (CET time :)) |
Yea it would have been better to get some of those PRs in first. Things seem to be moving along very quickly.
Thank youuu! |
There was a problem hiding this comment.
A quick first past by checking with a couple of occurrences forgotten elsewhere:
- In the function
partial_dependence, in the "See Also" section, we should refer to the.from_estimatormethod and remove the entry for theplot_partial_dependencefunction.
I will have a deeper look on the code itself now but since that all tests are passing, I think that there is no reason for it to be OK.
doc/developers/plotting.rst
Outdated
| @@ -64,7 +64,7 @@ Plotting with Multiple Axes | |||
| --------------------------- | |||
|
|
|||
| Some of the plotting tools like | |||
| :func:`~sklearn.inspection.plot_partial_dependence` and | |||
| :func:`~sklearn.inspection.PartialDependenceDisplay.from_estimator` and | |||
| :class:`~sklearn.inspection.PartialDependenceDisplay` support plottong on | |||
There was a problem hiding this comment.
There is another occurence of plot_partial_dependence in l.90
| @@ -34,13 +35,19 @@ def clf_diabetes(diabetes): | |||
| return clf | |||
|
|
|||
|
|
|||
| def test_plot_partial_dependence_deprecation(pyplot, clf_diabetes, diabetes): | |||
There was a problem hiding this comment.
Could you isolate all the test of the plot_partial_dependence in a py file. Following what I did for the other, the file would be name test_partial_dependence_display.py
Then you can let the test_plot_partial_dependence.py untouched, only adding a filter for the warnings on the top of the file:
pytestmark = pytest.mark.filterwarnings(
# TODO: Remove in 1.2 (as well as all the tests below)
"ignore:Function plot_partial_dependence is deprecated",
)| @@ -145,13 +145,13 @@ ICE plots: | |||
|
|
|||
There was a problem hiding this comment.
l.141, there is an occurence for the function plot_partial_dependence. We should instead refer to the class method .from_estimator
| with pytest.warns(FutureWarning): | ||
| plot_partial_dependence(clf_diabetes, diabetes.data, [0]) | ||
|
|
||
|
|
||
| @pytest.mark.filterwarnings("ignore:A Bunch will be returned") | ||
| @pytest.mark.parametrize("grid_resolution", [10, 20]) | ||
| def test_plot_partial_dependence(grid_resolution, pyplot, clf_diabetes, diabetes): |
There was a problem hiding this comment.
if possible, I would consider to rename the name of the test functions where plot_partial_dependence occur and replace by partial_dependence_display
| with multiple calls. To plot the the partial dependence for multiple | ||
| estimators, please pass the axes created by the first call to the | ||
| second call:: | ||
| :func:`PartialDependenceDisplay.from_estimator` does not support using |
There was a problem hiding this comment.
I think that we want to revert this part. We still want the documentation to be unchanged.
There was a problem hiding this comment.
We only need a .. deprecated:: 1.0 above the note mentioning to use the .from_estimator instead.
| @@ -267,181 +271,34 @@ def plot_partial_dependence( | |||
| >>> import matplotlib.pyplot as plt | |||
| >>> from sklearn.datasets import make_friedman1 | |||
| >>> from sklearn.ensemble import GradientBoostingRegressor | |||
| >>> from sklearn.inspection import plot_partial_dependence | |||
| >>> from sklearn.inspection import PartialDependenceDisplay | |||
There was a problem hiding this comment.
Same here. We want to revert. However, could you add .from_estimator in the "See Also" above.
| display = PartialDependenceDisplay( | ||
| pd_results=pd_results, | ||
| features=features, | ||
| return PartialDependenceDisplay.from_estimator( |
There was a problem hiding this comment.
For the other classes, @NicolasHug was in favor to keep the function untouched and instead to have a bit of duplicated code during the deprecation.
There was a problem hiding this comment.
Okay, updated PR with the function body and the tests duplicated.
| @@ -576,7 +433,6 @@ class PartialDependenceDisplay: | |||
| See Also | |||
| -------- | |||
| partial_dependence : Compute Partial Dependence values. | |||
There was a problem hiding this comment.
I would add as well the .from_estimator method at this level.
|
|
||
| Parameters | ||
| ---------- | ||
| estimator : BaseEstimator A fitted estimator object implementing |
There was a problem hiding this comment.
missing a breaking line after BaseEstimator
| not (0 <= target_idx < len(estimator.classes_)) | ||
| or estimator.classes_[target_idx] != target | ||
| ): | ||
| raise ValueError("target not in est.classes_, got {}".format(target)) |
| try: | ||
| fx = feature_names.index(fx) | ||
| except ValueError as e: | ||
| raise ValueError("Feature %s not in feature_names" % fx) from e |
| axes = np.asarray(ax, dtype=object) | ||
| if axes.size != len(features): | ||
| raise ValueError( | ||
| "Expected ax to have {} axes, got {}".format( |
| if i >= len(feature_names): | ||
| raise ValueError( | ||
| "All entries of features must be less than " | ||
| "len(feature_names) = {0}, got {1}.".format(len(feature_names), i) |
| raise ValueError("target must be specified for multi-output regressors") | ||
| if not 0 <= target <= n_tasks: | ||
| raise ValueError( | ||
| "target must be in [0, n_tasks], got {}.".format(target) |
| ... [1, 2]) # doctest: +SKIP | ||
| >>> disp2 = plot_partial_dependence(est2, X, [1, 2], | ||
| ... ax=disp1.axes_) | ||
| ... ax=disp1.axes_) # doctest: +SKIP |
There was a problem hiding this comment.
Added skips here to avoid the future warning.
| >>> plot_partial_dependence(clf, X, [0, (0, 1)]) # doctest: +SKIP | ||
| <...> | ||
| >>> plt.show() | ||
| >>> plt.show() # doctest: +SKIP |
There was a problem hiding this comment.
Added skips here to avoid the future warning.
| """Partial dependence (PD) and individual conditional expectation (ICE) | ||
| plots. |
There was a problem hiding this comment.
Can we succeed to make it on a single line (for future numpydoc validation) ;)
| :term:`predict_proba`, or :term:`decision_function`. | ||
| Multioutput-multiclass classifiers are not supported. | ||
|
|
||
| X : {array-like or dataframe} of shape (n_samples, n_features) |
There was a problem hiding this comment.
| X : {array-like or dataframe} of shape (n_samples, n_features) | |
| X : {array-like, dataframe} of shape (n_samples, n_features) |
|
|
||
| .. versionadded:: 1.0 |
There was a problem hiding this comment.
| .. versionadded:: 1.0 |
|
|
||
| .. versionadded:: 1.0 |
There was a problem hiding this comment.
| .. versionadded:: 1.0 |
|
|
||
| .. versionadded:: 0.22 |
There was a problem hiding this comment.
| .. versionadded:: 0.22 |
|
|
||
| .. versionadded:: 0.24 |
There was a problem hiding this comment.
| .. versionadded:: 0.24 |
|
|
||
| .. versionadded:: 0.24 |
There was a problem hiding this comment.
| .. versionadded:: 0.24 |
|
|
||
| .. versionadded:: 0.24 |
There was a problem hiding this comment.
| .. versionadded:: 0.24 |
|
pinging @adrinjalali and tagging #20965 This one would be great to be included. All the current displays would have the same API and the deprecation will be announced. However, we need a second review :) |
|
For reviewers, this PR looks huge, but it's mostly a copy and paste of the tests and the implementation. |
|
This is over 1.2k lines of code/change. I'm not sure if it's feasible to have it for the release. I'd like to treat this as a non-blocker, but happy to include it if it's merged by the time of the release. |
|
@glemaitre In trying to make this PR smaller, I defined a private Now most of the diff is the docstring of In the future, we can do another refactor to make this nicer. |
|
Fine with me.
|
|
Merging this, and will include in the release. |
|
Thanks @adrinjalali |
* API Deprecates plot_partial_dependence * DOC Adds whats new * DOC Fixes doc build errors * CLN Removes feature warning * CLN Address comments * DOC Fix docstrings * TST Skip future warning tests * DOC Adjust docstrings * REV Reduce diff * DOC Adds docstring
* API Deprecates plot_partial_dependence * DOC Adds whats new * DOC Fixes doc build errors * CLN Removes feature warning * CLN Address comments * DOC Fix docstrings * TST Skip future warning tests * DOC Adjust docstrings * REV Reduce diff * DOC Adds docstring
* API Deprecates plot_partial_dependence * DOC Adds whats new * DOC Fixes doc build errors * CLN Removes feature warning * CLN Address comments * DOC Fix docstrings * TST Skip future warning tests * DOC Adjust docstrings * REV Reduce diff * DOC Adds docstring
Deprecates
plot_partial_dependenceforPartialDependenceDisplay.from_estimator.The testing in
sklearn/inspection/_plot/tests/test_plot_partial_dependence.pyswitches to thefrom_estimatorversion and does not callplot_partial_dependence. I think this is okay given thatplot_partial_dependenceis a simple redirect to the class method.CC @glemaitre