[MRG] documentation for random_state in forest.py #15516
Conversation
|
Looks good to me pending CI |
|
@NicolasHug Let me know what you think. Thanks! |
|
Also @TomDLT or maybe @adrinjalali (seems to me that you volunteered to be pinged... ;) )... this PR looks pretty ready for merging? |
There was a problem hiding this comment.
This is a clear improvement, thanks @MDouriez . Made some comments but LGTM when addressed
sklearn/ensemble/_forest.py
Outdated
| Also note that the features are always randomly permuted at each split. | ||
| Therefore, the best found split may vary, even with the same training | ||
| data, ``max_features=n_features`` and ``bootstrap=False``, if the | ||
| improvement of the criterion is identical for several splits enumerated | ||
| during the search of the best split. To obtain a deterministic | ||
| behaviour during fitting, ``random_state`` has to be fixed. |
There was a problem hiding this comment.
I think we can omit this last part (from "Also note that..." to "has to be fixed").
The rest above is perfect
There was a problem hiding this comment.
This note was already there. I just moved it. Should I still remove it?
There was a problem hiding this comment.
I see, maybe just leave it where it was in the notes then
|
Should be ready for final review @NicolasHug @adrinjalali.
|
|
Also interestingly, |
Good catch, could you please open an issue for this? |
|
filed an issue #15670
|
* documentation for random_state in forests * move note to parameter * same for RandomForestRegressor * add doc for ExtraTreesRegressor and ExtraTreesClassifier * skip line * lint * move note back to where it was * add Glossary in RandomForestRegressor * adding description for RandomTreesEmbedding * small fix * correct description for RandomTreesEmbedding
* documentation for random_state in forests * move note to parameter * same for RandomForestRegressor * add doc for ExtraTreesRegressor and ExtraTreesClassifier * skip line * lint * move note back to where it was * add Glossary in RandomForestRegressor * adding description for RandomTreesEmbedding * small fix * correct description for RandomTreesEmbedding
* documentation for random_state in forests * move note to parameter * same for RandomForestRegressor * add doc for ExtraTreesRegressor and ExtraTreesClassifier * skip line * lint * move note back to where it was * add Glossary in RandomForestRegressor * adding description for RandomTreesEmbedding * small fix * correct description for RandomTreesEmbedding
Reference Issues/PRs
part of this issue: #15222
(related to #15264)
What does this implement/fix? Explain your changes.
This PR improves the documentation for
RandomForestClassifier,RandomForestRegressor,ExtraTreesClassifierandExtraTreesRegressorregarding the source of randomness. The only changes were made in the docstrings ofsklearn/ensemble/_forest.py.Any other comments?