-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[MRG] DOC User guide section for histogram-based GBDTs #14525
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.
I don't know much about the new gradient boosting implementation but it was an interesting read. A few comments are below.
Great work on this new implementation in general @NicolasHug !
Thanks @rth! Comments were addressed |
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.
LGTM, pending a second opinion on #14525 (comment)
Thanks for the reviews! I added a section d3e6ca0 with implementation details to explain why this implementation is faster. I think it can be useful for users for it to feel less like magic. WDYT? |
looks good :) |
for parallelization through Cython. The number of threads that is used can | ||
be changed using the ``OMP_NUM_THREADS`` environment variable. By default, | ||
all available cores are used. Please refer to the OpenMP documentation for | ||
details. |
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.
Link to OpenMP documentation?
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.
(Weird that the previous comments got lost, we discussed this with Adrin already)
I don't want to add it because the openmp doc is a 300 pages pdf. Googling OMP_NUM_THREADS gives you the docs from intel and ibm but they're probably not official.
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.
LGTM, should we merge or do you want another review since there's been a few changes?
I think we can merge, Thomas reviewed with the latest changes and I addressed his comments |
Long overdue, this PR adds a section in the user guide dedicated to the new GBDTs.
ping @adrinjalali @glemaitre @amueller @thomasjpfan