ENH compute histograms only for allowed features in HGBT #24856
Conversation
|
Summary: This PR clearly reduces fit time with interaction constraints (35% on Higgs benchmark) and no performance penalty without interactions. With Interaction ConstraintsNot interactions allowed. MAIN with commit 85a8aa6 with interaction constraintsthis PR with interaction constraintsWithout interaction constraintsMAINThis PR |
| has_interaction_cst = allowed_features is not None | ||
| if has_interaction_cst: | ||
| n_allowed_features = allowed_features.shape[0] | ||
|
|
||
| with nogil: |
There was a problem hiding this comment.
This allows reusing thread, eventually reducing OpenMP's overhead.
| with nogil: | |
| with nogil, parallel(num_threads=n_threads: |
There was a problem hiding this comment.
Could we try this out in another PR?
I did not touch those implementation parts and would like to keep it that way.
| for f_idx in prange( | ||
| n_allowed_features, schedule='static', num_threads=n_threads | ||
| ): |
There was a problem hiding this comment.
If the here-above suggested nogil, parallel-context is used, this num_threads must be removed here and in previous prange loops I can't suggest on.
| for f_idx in prange( | |
| n_allowed_features, schedule='static', num_threads=n_threads | |
| ): | |
| for f_idx in prange(n_allowed_features, schedule='static'): |
Note that this partern
There was a problem hiding this comment.
In another PR, maybe. See comment above.
| if has_interaction_cst: | ||
| feature_idx = allowed_features[f_idx] | ||
| else: | ||
| feature_idx = f_idx |
There was a problem hiding this comment.
Are there performances benefit of making the branching outside the prange loops and have one prange loop per branch?
There was a problem hiding this comment.
I tried that, but I did not observe any runtime difference. Usually, the number of features is small (less than 10, 100, maybe 1000), but sample size is large. The inner loop is then over n_samples and there branching should be avoided.
|
@jjerphan I hope it's ready now. All issues/merge conflicts are fixed. I also added a whatsnew entry. |
|
Still LGTM, yes. I just have labelled this PR as "Waiting for a Second Reviewer". |
|
Thanks @lorentzenchr! |
Reference Issues/PRs
Follow-up of #21020.
What does this implement/fix? Explain your changes.
This PR restricts the computation of histograms in
HistGradientBoostingRegressorandHistGradientBoostingClassifierto features that are allowed to be split on. This gives a boost in performance (fit time).Any other comments?