-
-
Notifications
You must be signed in to change notification settings - Fork 26k
DOC Fix doc of defaults in grower.py #18206
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @franslarsson for the PR!
I have just a few suggestions.
@@ -508,7 +513,7 @@ def make_predictor(self, bin_thresholds=None): | |||
|
|||
Parameters | |||
---------- | |||
bin_thresholds : array-like of floats, optional (default=None) | |||
bin_thresholds : array-like of floats, default=None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bin_thresholds : array-like of floats, default=None | |
bin_thresholds : array-like, dtype=float, default=None |
Co-authored-by: Juan Carlos Alfaro Jiménez <JuanCarlos.Alfaro@uclm.es>
Co-authored-by: Juan Carlos Alfaro Jiménez <JuanCarlos.Alfaro@uclm.es>
Co-authored-by: Juan Carlos Alfaro Jiménez <JuanCarlos.Alfaro@uclm.es>
Thanks for the review @alfaro96 . I have committed the changes for unsigned int and bool. However, regarding the dtype for float and int I am a bit unsure if we should use the valid numpy dtypes instead, e.g. np.float64, np.float32, np.int64 as suggested in the comments in #18025 . And if so would that be the case for array-like as well? |
NumPy dtype will apply only to |
Merge origin doc_grower into local doc_grower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@franslarsson Could you make the 2 changes. Otherwise LGTM
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
I have now made the changes. Thanks for the review @glemaitre and @alfaro96 . |
Thanks @franslarsson |
…18206) Co-authored-by: Juan Carlos Alfaro Jiménez <JuanCarlos.Alfaro@uclm.es> Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Reference Issues/PRs
See #15761
What does this implement/fix? Explain your changes.
This PR change how default values are documented in sklearn.ensemble._hist_gradient_boosting.grower.py and update docstring according to guideline.
Any other comments?
Added missing parameter monotonic_cst in docstring for TreeGrower.