Support column split in multi-target hist
#9171
Conversation
|
@trivialfis @hcho3 any idea on the test failures on macos-11? I can't reproduce it on my linux box. |
|
Unfortunately, I don't have a macos device. |
|
Let me try to reproduce it on my Macbook. |
|
I started a mac instance on AWS and was able to reproduce the test failures. I think I know where the failures are coming from, but still need to do some debugging to figure out a fix. |
|
@trivialfis finally got the CI to pass. Please take a look. |
| @@ -371,7 +369,8 @@ auto AddCategories(std::set<float> const &categories, HistogramCuts *cuts) { | |||
| InvalidCategory(); | |||
| } | |||
| auto &cut_values = cuts->cut_values_.HostVector(); | |||
| auto max_cat = *std::max_element(categories.cbegin(), categories.cend()); | |||
There was a problem hiding this comment.
@trivialfis I think this is a legit bug. If categories is empty, std::max_element returns categories.cend, which leads to undefined behavior when accessed.
Presumably with row-split categories would never be empty, but with column-split it's pretty common.
There was a problem hiding this comment.
Would be great if you can leave a comment there so that others don't accidentally remove it.
| @@ -698,6 +698,9 @@ void MetaInfo::Extend(MetaInfo const& that, bool accumulate_rows, bool check_col | |||
| this->feature_type_names = that.feature_type_names; | |||
| auto &h_feature_types = feature_types.HostVector(); | |||
| LoadFeatureType(this->feature_type_names, &h_feature_types); | |||
| } else if (!that.feature_types.Empty()) { | |||
There was a problem hiding this comment.
@rongou Could you please help comment on when the meta info has feature types but doesn't have type names?
There was a problem hiding this comment.
I think in some tests we set the feature types directly without setting the type names.
There was a problem hiding this comment.
Got it, let me try to run the tests.
Also added support for categorical features in
approx.The main challenge is that the expand entry objects contain vector fields, which means the memory is not contiguous, so we cannot allgather the objects in one go. We have to allgather the primitive fields first, then separately allgather the vector fields.
An alternative may be to serialize the expand entry objects into a binary blob/json, do allgather, then deserialize them back. But that might have a bigger performance impact.
Future work may be to push
AllgatherVinto the communicator. I think we can implement it more efficiently in gRPC.