-
-
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
Model IO in JSON. #5110
Model IO in JSON. #5110
Conversation
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. I love how the JSON object abstraction lets us write serialization succinctly and clearly. See my minor comments.
} | ||
|
||
{ | ||
model = Json::Load({model_str.c_str(), model_str.size()}); |
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.
Nit: It took me a little bit to realize that StringView object was being implicitly constructed. Personally I'd prefer declaring StringView constructor to be explicit
.
|
||
{ | ||
model = Json::Load({model_str.c_str(), model_str.size()}); | ||
model = model["model"]; |
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.
Question: Is this simply a shallow copy (pointer assignment), or is this involving a deep copy?
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.
@hcho3 It involves a deep copy. Use reference if you don't want a copy
{ | ||
model = Json::Load({model_str.c_str(), model_str.size()}); | ||
model = model["model"]; | ||
auto weights = get<Array>(model["weights"]); | ||
ASSERT_EQ(weights.size(), 17); // 16 + 1 (bias) | ||
} |
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.
Isn't this block a duplicate of line 38-45?
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.
@hcho3 No, the model is reloaded. But I will structure it more clearly.
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.
So the model is loaded from model_str
twice?
auto& j_model = model["model"]; | ||
model["parameters"] = Object(); | ||
|
||
gbm->SaveModel(&j_model); |
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.
Nice, this snippet is a good exhibit of the power and flexibility of a nested key-value store.
for (int32_t iter = 0; iter < kIters; ++iter) { | ||
learner->UpdateOneIter(iter, p_dmat.get()); | ||
} | ||
learner->SetAttr("bset_score", "15.2"); |
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.
Did you mean base_score
?
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.
@hcho3 Typo, opening a new PR for addressing your comments.
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.
The "model" part of #4732. Follow up PRs will be for "Configuration".