Skip to content

FIX Enable setting of sub-parameters for deprecated base_estimator param #25477

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Feb 9, 2023
Merged

FIX Enable setting of sub-parameters for deprecated base_estimator param #25477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
698ddac
Enable setting of sub-parameters for base_estimator param
betatim Jan 25, 2023
3d6d13c
Raise a warning when using old parameter names
betatim Jan 26, 2023
2b13d8b
Add what's new entries
betatim Jan 26, 2023
88407d5
Merge branch 'main' into fix-25470
betatim Feb 8, 2023
a331227
Move what's new entries, add TODO marker
betatim Feb 8, 2023
89ee48f
Merge branch 'main' into fix-25470
betatim Feb 8, 2023
8be004e
Fix missing newline
betatim Feb 8, 2023
5c4b441
Fix what's new syntax
betatim Feb 8, 2023
d20c386
Merge branch 'main' into fix-25470
jeremiedbb Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions doc/whats_new/v1.2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ Changes impacting all modules
Changelog
---------

:mod:`sklearn.calibration`
..........................

- |Fix| A deprecation warning is raised when using the `base_estimator__` prefix
to set parameters of the estimator used in :class:`calibration.CalibratedClassifierCV`.
:pr:`25477` by :user:`Tim Head <betatim>`.

:mod:`sklearn.cluster`
......................

Expand All @@ -41,6 +48,15 @@ Changelog
empty selection of columns when `set_output(transform="pandas")`.
:pr:`25570` by `Thomas Fan`_.

:mod:`sklearn.ensemble`
.......................

- |Fix| A deprecation warning is raised when using the `base_estimator__` prefix
to set parameters of the estimator used in :class:`ensemble.AdaBoostClassifier`,
:class:`ensemble.AdaBoostRegressor`, :class:`ensemble.BaggingClassifier`,
and :class:`ensemble.BaggingRegressor`.
:pr:`25477` by :user:`Tim Head <betatim>`.

:mod:`sklearn.isotonic`
.......................

Expand Down
19 changes: 19 additions & 0 deletions sklearn/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,25 @@ def set_params(self, **params):
valid_params[key] = value

for key, sub_params in nested_params.items():
# TODO(1.4): remove specific handling of "base_estimator".
# The "base_estimator" key is special. It was deprecated and
# renamed to "estimator" for several estimators. This means we
# need to translate it here and set sub-parameters on "estimator",
# but only if the user did not explicitly set a value for
# "base_estimator".
if (
key == "base_estimator"
and valid_params[key] == "deprecated"
and self.__module__.startswith("sklearn.")
):
warnings.warn(
f"Parameter 'base_estimator' of {self.__class__.__name__} is"
Copy link
Member

Choose a reason for hiding this comment

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

Not actionable: It would be harder for third party estimators to deprecate a positional parameter because they would also need adjust set_params to still work with the previous name.

" deprecated in favor of 'estimator'. See"
f" {self.__class__.__name__}'s docstring for more details.",
FutureWarning,
stacklevel=2,
)
key = "estimator"
valid_params[key].set_params(**sub_params)

return self
Expand Down
18 changes: 18 additions & 0 deletions sklearn/ensemble/tests/test_weight_boosting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,3 +630,21 @@ def test_base_estimator_property_deprecated(AdaBoost):
)
with pytest.warns(FutureWarning, match=warn_msg):
model.base_estimator_


# TODO(1.4): remove in 1.4
def test_deprecated_base_estimator_parameters_can_be_set():
"""Check that setting base_estimator parameters works.

During the deprecation cycle setting "base_estimator__*" params should
work.

Non-regression test for https://github.com/scikit-learn/scikit-learn/issues/25470
"""
# This implicitly sets "estimator", it is how old code (pre v1.2) would
# have instantiated AdaBoostClassifier and back then it would set
# "base_estimator".
clf = AdaBoostClassifier(DecisionTreeClassifier())
Copy link
Member

Choose a reason for hiding this comment

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

actually here we are setting estimator and not base_estimator. It is completely fine that it works. However, could we parametrize the non-regression test to check that both works when setting estimator and base_estimator.

It could also be work to call fit just to make sure that there is not latter side-effect.

Copy link
Member

@ogrisel ogrisel Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this test is enough. If clf = AdaBoostClassifier(base_estimator=DecisionTreeClassifier()) then I would expect that the warning would be raised at __init__ fit time and no warning would be raised when calling clf.set_params(base_estimator__max_depth=2).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is indeed what happens. I also think the warning at fit time is enough in this case.


with pytest.warns(FutureWarning, match="Parameter 'base_estimator' of"):
clf.set_params(base_estimator__max_depth=2)