Skip to content
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

[python] add parameter object_hook to method dump_model #4533

Merged
merged 6 commits into from
Aug 23, 2021

Conversation

xadupre
Copy link
Member

@xadupre xadupre commented Aug 19, 2021

Method dump_model parses a string to create a JSON structure. If a function needs to modify this structure, it needs to walk through the whole structure a second time. The object_hook can be used to do both in one pass. This trick was used to significantly speed up the conversion of an heavy model (>40.000 trees) into ONNX (onnx/onnxmltools#491). This PR would avoid duplication of the code of dump_model in onnxmltools.

@xadupre xadupre changed the title add parameter object_hook to method dump_model (python API) [python] add parameter object_hook to method dump_model Aug 19, 2021
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xadupre Thank you very much for the contribution!
Very interesting addition! TBH, I've never used this argument of json.loads().
Impressive results in the ONNX speedup! I believe new argument might be useful for other users and it's worth exposing object_hook in the dump_model() signature.

Generally LGTM, just one comment about numpy doc style below.

python-package/lightgbm/basic.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for this! Really nice contribution and we appreciate you taking the time to upstream it into LightGBM.

I agree with @StrikerRUS , this is a great addition and just needs a slight documentation update

@xadupre
Copy link
Member Author

xadupre commented Aug 19, 2021

Thanks for the feedback. I improved the documentation.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! LGTM, except one minor nit about emphasizing style in the docstring.

Ah, forgot to say special thanks for adding test! 🙂

python-package/lightgbm/basic.py Outdated Show resolved Hide resolved
Co-authored-by: Nikita Titov <nekit94-08@mail.ru>
@StrikerRUS StrikerRUS merged commit 11d7608 into microsoft:master Aug 23, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants