MAINT rename base_estimator and base_estimator_ in ensemble classes
#23819
Conversation
…or has decision_function
|
Hi @EdAbati , thanks for your pull request! And thanks for giving a try to my first suggestion! I like your solution! :D |
|
Hi @cmarmo , thank you very much for the fast review. |
Co-authored-by: Guillaume Lemaitre <[email protected]>
|
@glemaitre I think I added all your suggestion :) |
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
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.
|
ping @jeremiedbb @thomasjpfan |
glemaitre
changed the title
base_estimator and base_estimator_ in ensemble classbase_estimator and base_estimator_ in ensemble classes
|
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
left a comment
jeremiedbb
left a comment
There was a problem hiding this comment.
Thanks for the PR @EdAbati ! Here are some comments/questions.
|
Hi @jeremiedbb , thank you for the review! I should have addressed everything. I have added a fix to the documentation of |
|
Nice thanks @EdAbati |
…n ensemble classes (scikit-learn#23819) Co-authored-by: Adrian Trujillo Duron <[email protected]> Co-authored-by: Guillaume Lemaitre <[email protected]>
5 participants
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
BaseBaggingas initially suggested by @cmarmo #9104, because bothBaggingRegressorandBaggingClassifierdepend on it. With this, I had to make a small update in the__init__of theIsolationForesttoo, since it is also subclass ofBaseBagging(I believe I should not make a deprecation cycle here, becauseestimatoris not a argument ofIsolationForest, 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