-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Add linear leaf models to json output (fixes #4186) #4329
Conversation
src/io/tree.cpp
Outdated
str_buf << "\"tree_structure\":{" << "\"leaf_value\":" << leaf_value_[0] << "}" << '\n'; | ||
if (is_linear_) { | ||
str_buf << "\"tree_structure\":{" << "\"leaf_value\":" << leaf_value_[0] << ", " << "\n"; | ||
str_buf << LinearModelToJSON(0); |
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 seems that the right bracket }
is missing
params['linear_tree'] = True | ||
train_data = lgb.Dataset(X, label=y) | ||
bst = lgb.train(params, train_data, num_boost_round=5) | ||
bst.dump_model(5, 0) |
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.
Shall we use loads
to validate that the output JSON content?
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 is done already in dump_model
:
LightGBM/python-package/lightgbm/basic.py
Line 3090 in 36957ed
ret = json.loads(string_buffer.value.decode('utf-8')) |
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.
@btrotta Thanks! LGTM.
@@ -411,3 +411,18 @@ def test_list_to_1d_numpy(y, dtype): | |||
result = lgb.basic.list_to_1d_numpy(y, dtype=dtype) | |||
assert result.size == 10 | |||
assert result.dtype == dtype | |||
|
|||
|
|||
def test_dump_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.
Thanks a lot for this enhancement!
Please move this test into test_engine.py
file because lgb.train()
function is from engine.py
module.
Also I think it will be useful to add some asserts into this test. Something like assert 'leaf_coeff' in dumped_model
for linear model and assert 'leaf_coeff' not in dumped_model
for ordinary one.
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.
Thank you very much! LGTM! Just more asserts for better coverage.
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
Restarted Appveyor because there was unrelated network issue. |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
As requested here: #3299 (comment)