-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Support categorical split in tree model dump. #7036
Conversation
993141e
to
b223b53
Compare
Codecov Report
@@ Coverage Diff @@
## master #7036 +/- ##
=======================================
Coverage 81.86% 81.87%
=======================================
Files 13 13
Lines 3916 3917 +1
=======================================
+ Hits 3206 3207 +1
Misses 710 710
Continue to review full report at Codecov.
|
src/tree/tree_model.cc
Outdated
auto name = split_index < fmap_.Size() | ||
? fmap_.Name(split_index) | ||
: "feat" + std::to_string(split_index); |
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.
Since name
is only needed for the error message, can you put this inside if (!is_categorical)
block? Inside, use LOG(FATAL)
to throw error
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.
Also, how come we are not writing anything to result
for case FeatureMap::kCategorical:
, like all the other cases?
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 kept the possibility that users don't provide a feature map with categorical type. Categorical is a different split type, all the others are just variations of numerical splits. So no matter what the feature map says it has to go through a dedicated print procedure.
I can restrict it and ensure a correct feature map is provided.
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 remove this logic, now users either not provide a feature map or it has to be consistent with the training data.
@hcho3 @RAMitchell could you please take a look at how the output should look. I'm currently printing the categories as sets, but we don't have an
|
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, the format looks fine to me.
e753e78
to
8d49052
Compare
Hmm .. gpu coordinate descent is failing non-deterministically.. |
Support categorical tree node for text dump.
Related #6503 .
Close #6298 .