MAINT rename `base_estimator` and `base_estimator_` in ensemble classes #23819

EdAbati · 2022-07-02T10:03:36Z

Reference Issues/PRs

Closes #9104. See also #22145.
Closes #22145

What does this implement/fix? Explain your changes.

I continued the stalled PR #22145.
I decided to change BaseBagging as initially suggested by @cmarmo #9104, because both BaggingRegressor and BaggingClassifier depend on it. With this, I had to make a small update in the __init__ of the IsolationForest too, since it is also subclass of BaseBagging (I believe I should not make a deprecation cycle here, because estimator is not a argument of IsolationForest, but please correct me if I am wrong).

I should have addressed everything in the comments, but please let me know if I missed anything. :)

Any other comments

With this PR all the classes mentioned here #9104 (comment) should be renamed and (I think) #9104 can be closed

…or has decision_function

cmarmo · 2022-07-02T20:22:07Z

Hi @EdAbati , thanks for your pull request! And thanks for giving a try to my first suggestion! I like your solution! :D
Do you mind checking in the documentation (the .rst files) if there is any occurrence of base_estimator for Bagging and replacing with estimator? I found one in glossary.rst (line 598) but maybe there are some more. Thanks!

EdAbati · 2022-07-02T20:41:40Z

Hi @cmarmo , thank you very much for the fast review.
docs should be updated now! :)

glemaitre

In addition, we should add a test for the deprecation of base_estimator_ in test_iforest.py.

doc/whats_new/v1.2.rst

sklearn/ensemble/tests/test_bagging.py

sklearn/ensemble/tests/test_weight_boosting.py

sklearn/ensemble/tests/test_forest.py

sklearn/ensemble/tests/test_base.py

sklearn/ensemble/tests/test_bagging.py

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

EdAbati · 2022-09-07T21:40:17Z

@glemaitre I think I added all your suggestion :)

sklearn/ensemble/tests/test_weight_boosting.py

sklearn/ensemble/tests/test_iforest.py

sklearn/ensemble/tests/test_forest.py

sklearn/ensemble/tests/test_bagging.py

glemaitre

I applied my small nitpick suggestion to be inline with our own way to find the deprecation to remove in the future.

On my side, this PR is good to go. We will need a second reviewer.

glemaitre · 2022-09-08T09:09:56Z

ping @jeremiedbb @thomasjpfan

EdAbati · 2022-09-08T09:54:35Z

Thank you very much @glemaitre for the help. :)

And apologies for the long back-and-forth, I got a bit confused on what needed to be done. (I hope I didn't make you waste too much time.)

jeremiedbb

Thanks for the PR @EdAbati ! Here are some comments/questions.

sklearn/ensemble/_base.py

sklearn/ensemble/_bagging.py

sklearn/ensemble/tests/test_bagging.py

EdAbati · 2022-09-08T21:27:00Z

Hi @jeremiedbb , thank you for the review! I should have addressed everything.

I have added a fix to the documentation of [RandomTreesEmbedding 2cd0cf2. The docstring said that the estimator used was a ExtraTreeClassifier when it is instead a ExtraTreeRegressor.

jeremiedbb

LGTM. Thanks @EdAbati

glemaitre · 2022-09-09T09:42:50Z

Nice thanks @EdAbati

…n ensemble classes (scikit-learn#23819) Co-authored-by: Adrian Trujillo Duron <adrian.td96@gmail.com> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

trujillo9616 and others added 18 commits January 7, 2022 15:08

Modified parameters in BaseBagging class

3d4ab73

Modified parameters in BaggingClassifier class

e9f1f80

Modified parameters in BaggingRegressor class

b73d98e

Modified parameters in tests

effb374

Added 2 new tests to check warning

9e6dbd1

Updated changelog

6cc6264

Reversed changes

e09a6c9

Modified BaggingClassifier and BaggingRegressor

6b3f8e3

Added tests for deprecration

2a8f019

reverted changelog

e171840

Merge remote-tracking branch 'upstream/main' into rename-base-estimator

6ad10e7

increased deprecated version in docstring

24eac4c

increased deprecated version in fit

3d1d07a

extracted _validate_n_estimators in separate method

ea2ec0f

change base_estimator to estimator in BaseBagging

ef6996a

updated warning message in tests

8b4a724

removed set_params

05b5af8

updated parameters test

7b59a0f

github-actions bot added the module:ensemble label Jul 2, 2022

EdAbati added 7 commits July 2, 2022 12:15

updated changelog

d844759

updated _estimator_has to support self.estimator

55c3987

renamed base_estimator arguments in test_base

e312114

added docstring to fit methods

f2a3324

added todo comments for deprecation

6553856

added test to check if not fitted BaggingClassifier with base_estimat…

3efe77c

…or has decision_function

fixed _estimator_has docstring

e8467f5

updated docs where base_estimator was mentioned

91f5581

cmarmo mentioned this pull request Jul 3, 2022

base_estimator vs estimator for meta-estimators #9104

Closed

glemaitre reviewed Sep 7, 2022

View reviewed changes

glemaitre changed the title ~~MAINT rename base_estimator to estimator in BaseBagging, BaggingRegressor and BaggingClassifier~~ MAINT rename base_estimator and base_estimator_ in ensemble class Sep 7, 2022

glemaitre added this to the 1.2 milestone Sep 7, 2022

glemaitre and others added 4 commits September 7, 2022 11:43

just to make the test pass

4e6831f

Update doc/whats_new/v1.2.rst

13dfe6f

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

adjusted deprecation test

0988741

adjusted all deprecation tests

97cb525

fixed test warning

2885e69

glemaitre reviewed Sep 8, 2022

View reviewed changes

Apply suggestions from code review

01a2a0a

glemaitre approved these changes Sep 8, 2022

View reviewed changes

glemaitre changed the title ~~MAINT rename base_estimator and base_estimator_ in ensemble class~~ MAINT rename base_estimator and base_estimator_ in ensemble classes Sep 8, 2022

jeremiedbb reviewed Sep 8, 2022

View reviewed changes

EdAbati added 4 commits September 8, 2022 22:42

minor fixes

a9b2e23

added test for _validate_estimator

99262ad

Merge remote-tracking branch 'upstream/main' into rename-base-estimator

3a8e538

fixed documentation RandomTreesEmbedding

2cd0cf2

cmarmo added the Waiting for Reviewer label Sep 9, 2022

jeremiedbb approved these changes Sep 9, 2022

View reviewed changes

jeremiedbb merged commit 306608e into scikit-learn:main Sep 9, 2022

EdAbati deleted the rename-base-estimator branch September 9, 2022 11:41

markotoplak mentioned this pull request Apr 21, 2023

AdaBoost: deprecation of "base_estimator" does not handle "base_estimator=None" setting properly #26241

Closed

markotoplak mentioned this pull request Sep 21, 2023

Allow scikit-learn 1.3 biolab/orange3#6585

Merged

glemaitre mentioned this pull request Aug 16, 2024

base_estimator in Chain classes while estimator is the convention in Bagging and MultiOutput classes? #29620

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT rename `base_estimator` and `base_estimator_` in ensemble classes #23819

MAINT rename `base_estimator` and `base_estimator_` in ensemble classes #23819

EdAbati commented Jul 2, 2022 •

edited by thomasjpfan

Loading

cmarmo commented Jul 2, 2022

EdAbati commented Jul 2, 2022

glemaitre left a comment

EdAbati commented Sep 7, 2022

glemaitre left a comment

glemaitre commented Sep 8, 2022

EdAbati commented Sep 8, 2022

jeremiedbb left a comment

EdAbati commented Sep 8, 2022

jeremiedbb left a comment

glemaitre commented Sep 9, 2022

MAINT rename base_estimator and base_estimator_ in ensemble classes #23819

MAINT rename base_estimator and base_estimator_ in ensemble classes #23819

Conversation

EdAbati commented Jul 2, 2022 • edited by thomasjpfan Loading

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments

cmarmo commented Jul 2, 2022

EdAbati commented Jul 2, 2022

glemaitre left a comment

Choose a reason for hiding this comment

EdAbati commented Sep 7, 2022

glemaitre left a comment

Choose a reason for hiding this comment

glemaitre commented Sep 8, 2022

EdAbati commented Sep 8, 2022

jeremiedbb left a comment

Choose a reason for hiding this comment

EdAbati commented Sep 8, 2022

jeremiedbb left a comment

Choose a reason for hiding this comment

glemaitre commented Sep 9, 2022

MAINT rename `base_estimator` and `base_estimator_` in ensemble classes #23819

MAINT rename `base_estimator` and `base_estimator_` in ensemble classes #23819

EdAbati commented Jul 2, 2022 •

edited by thomasjpfan

Loading