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
CPU evaluation for cat data. #7393
Conversation
* Implementation for one hot based. * Implementation for partition based.
@@ -5,6 +5,13 @@ | |||
#include "../../../src/common/quantile.cuh" | |||
|
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.
Some issues in the quantile test were found after changing the SimpleLCG
, so the changes in this file are related.
@@ -91,49 +96,118 @@ template <typename GradientSumT, typename ExpandEntry> class HistEvaluator { | |||
iend = static_cast<int32_t>(cut_ptr[fidx]) - 1; | |||
} | |||
|
|||
auto calc_bin_value = [&](auto i) { |
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.
This can be split into multiple functions, but then we will have lots of duplicated code.
std::vector<size_t> sorted_idx(n_bins); | ||
std::iota(sorted_idx.begin(), sorted_idx.end(), 0); | ||
auto feat_hist = histogram.subspan(cut_ptr[fidx], n_bins); | ||
std::stable_sort(sorted_idx.begin(), sorted_idx.end(), [&](size_t l, size_t r) { |
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.
Can't use argsort as we don't have a cpu transform iter.
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
@@ -39,7 +36,7 @@ template <typename GradientSumT> void TestEvaluateSplits() { | |||
std::iota(row_indices.begin(), row_indices.end(), 0); | |||
row_set_collection.Init(); | |||
|
|||
auto hist_builder = GHistBuilder<GradientSumT>(n_threads, gmat.cut.Ptrs().back()); | |||
auto hist_builder = GHistBuilder<GradientSumT>(omp_get_max_threads(), gmat.cut.Ptrs().back()); |
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.
Shouldn't we call function OmpGetThreadLimit()
to query OMP_THREAD_LIMIT
?
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.
It's a c++ unittest, I think we don't have to worry about that too much. But on the other hand, we need more thorough integration tests for that env, probably in another PR.
size_t n_cats{8}; | ||
|
||
auto dmat = | ||
RandomDataGenerator(kRows, kCols, 0).Seed(3).Type(ft).MaxCategory(n_cats).GenerateDMatrix(); |
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 really like how we can generate random data with a fluent interface.
Currently, the evaluation function is using parameters from numerical split. We should set up a new set of training parameters later. But before that, I want to have a working implementation and some experiment results first.
Extracted from #7214 .