Skip to content

DOC add additional pointers in Forest regarding how to use warm_start #24579

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.

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 8 commits into from
Nov 13, 2022

Conversation

choo8
Copy link

@choo8 choo8 commented Oct 4, 2022

I created the fixes according to the comments made by @NicolasHug #20435 (comment). It's my first time contributing to an open source project, so please let me know if there is anything I can do to improve my pull request. Thank You!

Reference Issues/PRs

Fixes #20435

What does this implement/fix? Explain your changes.

Removes unnecessary OOB computation when n_more_estimators == 0 and updates the docs for BaseForest-derived classes to link to https://scikit-learn.org/stable/modules/ensemble.html#fitting-additional-weak-learners

Any other comments?

@choo8 choo8 changed the title Edited docs for warm_start and removed OOB computation for n_estimato… [MRG] Edited docs for warm_start and removed OOB computation for n_estimato… Oct 6, 2022
@cmarmo
Copy link
Contributor

cmarmo commented Oct 8, 2022

Hi @choo8, thanks for your pull request.
I believe a changelog is not needed as the modifications are not supposed to change the behavior of the classes.
If I understand correctly, the suggestions are applied, so this looks good to me.
Something went wrong in the documentation build: do you mind pushing an empty commit or synchronize with main to retrigger it? Thanks!

@choo8
Copy link
Author

choo8 commented Oct 9, 2022

Hi @cmarmo, I synchronized the pull request with the latest commits on the main branch and passed the tests. Please let me know if there is anything else I should do to get this pull request merged. Thanks!

@cmarmo
Copy link
Contributor

cmarmo commented Oct 9, 2022

Thanks @choo8, all is green! :)
Now we should wait for a core-dev review... please be patient... 🙏

Comment on lines 497 to 501
# Only perform OOB computation again if there are newly grown trees or
# if it was not previously calculated due to oob_score parameter being False
if self.oob_score and (
n_more_estimators > 0 or not hasattr(self, "oob_score_")
):
Copy link
Member

Choose a reason for hiding this comment

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

Could we isolate this change in another PR? It will require writing some new unit tests. Since the documentation would be mergeable as-it, it would be the best way to go.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @glemaitre, thank you for your comments. As suggested, I have updated my PR to remove the changes to the OOB computation and only kept the changes to the documentation.

@glemaitre glemaitre changed the title [MRG] Edited docs for warm_start and removed OOB computation for n_estimato… DOC add additional pointers in Forest regarding how to use warm_start Nov 3, 2022
@glemaitre glemaitre merged commit 980ded1 into scikit-learn:main Nov 13, 2022
@glemaitre
Copy link
Member

Thanks @choo8 LGTM. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect documentation for warm_start behavior on BaseForest-derived classes
3 participants