[MRG] ENH add option to set the number of OpenMP threads in HistGBDT #16070
Conversation
|
ping @ogrisel @NicolasHug @jeremiedbb I think that this could be a draft to check if it was what you had in mind. |
|
To be in line with #14265, we could avoid exposing ( |
|
Sorry, didn't mean to close. Indeed the parameter should not be exposed. We should only use As documented in #15116, users should use OMP_NUM_THREADS |
I am fine with this. @NicolasHug could you comment on the original issue to know why @ogrisel wanted to expose it. |
|
OK so it should be really [MRG] now. @NicolasHug do you want to have a look. |
sklearn/ensemble/_hist_gradient_boosting/tests/test_splitting.py
Outdated
Show resolved
Hide resolved
sklearn/ensemble/_hist_gradient_boosting/tests/test_splitting.py
Outdated
Show resolved
Hide resolved
|
@NicolasHug I remove the fixture |
|
@glemaitre what's the status of this PR? It currently adds a |
|
We spoke with @ogrisel last week and in fact, we will need to make more
benchmarks to see if there is an effect with n_samples and the best
n_threads to be sure that we don't mess up the default.
It currently adds a n_threads param but we haven't decided on that yet
It was a proposal nothing definitive
…On Sun, 23 Feb 2020 at 15:36, Nicolas Hug ***@***.***> wrote:
@glemaitre <https://github.com/glemaitre> what's the status of this PR?
It currently adds a n_threads param but we haven't decided on that yet
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16070?email_source=notifications&email_token=ABY32PYH26E36FZXDK3XRV3REKCXPA5CNFSM4KEWWMYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMV5J5Q#issuecomment-590075126>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABY32PYHHEGUWYBIL6VKF23REKCXPANCNFSM4KEWWMYA>
.
--
Guillaume Lemaitre
Scikit-learn @ Inria Foundation
https://glemaitre.github.io/
|
|
So this is stalled? Should we tag with "Needs Bencmarks"? |
|
This is true that this a pattern that we need in several PR/issues.
…On Sun, 23 Feb 2020 at 16:14, Nicolas Hug ***@***.***> wrote:
So this is stalled? Should we tag with "Needs Bencmarks"?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16070?email_source=notifications&email_token=ABY32P6632ALXB2DTVG6PUTREKHF5A5CNFSM4KEWWMYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMV6GIA#issuecomment-590078752>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABY32P6LPDZV5BGK52ZFTOTREKHF5ANCNFSM4KEWWMYA>
.
--
Guillaume Lemaitre
Scikit-learn @ Inria Foundation
https://glemaitre.github.io/
|
|
I mean, we do have the tag. I'm trying to keep other reviewers from needlessly spending time reviewing the PR |
|
Oh good I did not know that we add a tag. Thanks. |
|
I believe this was fixed concurrently in #20477. Let's close. |
closes #16016
Add a
n_jobsparameter to set the number of threads (OpenMP threads) to be used when finding split. By default, we limit to 16 threads to avoid oversubscription issue.