Using column_sampler for optimization of ColWiseBuildHist #8319
Conversation
|
Please ignore the failing MacOS tests from GitHub Actions. We're working to fix it. |
|
Hi @trivialfis , |
|
Apologies for the slow response, I will get to this tomorrow. |
Hi, |
There was a problem hiding this comment.
Apologies for the delay. May I ask the specific objective of this PR? Are you trying to optimize the case to build for less columns when sampling is enabled?
I'm concerned with optimizations that don't generalize and requires edge cases. I haven't been able to track which test can confirm the correctness of the code.
| @@ -284,13 +300,14 @@ void ColsWiseBuildHistKernel(const std::vector<GradientPair> &gpair, | |||
| }; | |||
|
|
|||
| const size_t n_features = gmat.cut.Ptrs().size() - 1; | |||
| const size_t n_columns = n_features; | |||
| const size_t n_columns = kColumnSampling ? fids.size() : n_features; | |||
There was a problem hiding this comment.
Is fids empty if there's no sampling?
There was a problem hiding this comment.
Fids is empty is case of condition here is false.
| @@ -76,6 +84,20 @@ class HistogramBuilder { | |||
| buffer_.Reset(this->n_threads_, n_nodes, space, target_hists); | |||
| } | |||
|
|
|||
| constexpr float kColsampleTh = 0.1; | |||
| bool column_sampling = (column_sampler_ != nullptr) && | |||
There was a problem hiding this comment.
When column sampler is nullptr?
There was a problem hiding this comment.
I set it to nullptr in tests of the histogram builder without using this optimization.
| @@ -76,6 +84,20 @@ class HistogramBuilder { | |||
| buffer_.Reset(this->n_threads_, n_nodes, space, target_hists); | |||
| } | |||
|
|
|||
| constexpr float kColsampleTh = 0.1; | |||
There was a problem hiding this comment.
It is an ad-hoc threshold value.
| constexpr float kColsampleTh = 0.1; | ||
| bool column_sampling = (column_sampler_ != nullptr) && | ||
| (train_param_.colsample_bytree < kColsampleTh || | ||
| train_param_.colsample_bylevel < kColsampleTh); |
There was a problem hiding this comment.
how about bynode? Is it used?
There was a problem hiding this comment.
Not now, maybe later, one can investigate these options more deeply.
yes, that was the idea
It is reasonable. I added a subcase for |
|
Let me try to look deeper into this PR. The optimization is definitely important but I would like to keep lesser conditional branches than it currently has. |
Hi,
this PR is a part of #7192.
It continues the optimization of BuildHistKernel, which was started at #8233.
In #8233 I removed the optimization related to column_sampler to simplify the reviewing process. Here I return it.
The methodology of the benchmarking is the same as in #8218. The only dataset affected by optimization is
santander. Training speed forsantanderincreases twice after applying this optimization. You can find the exact numbers at #8233.I am looking forward for your review and comments!