LightGBM icon indicating copy to clipboard operation
LightGBM copied to clipboard

WIP: Load back parameters from saved model file (fixes #2613)

Open zyxue opened this issue 2 years ago • 8 comments

This is still just an idea to collect feedbacks.

The approach in this PR:

  1. made loaded_parameter_ an attribute of Boosting instead of GBDT so it can be accessed in c_api.cpp.
  2. implemented LGBM_BoosterGetConfig to return loaded_parameter_ as a string.
  3. loaded_parameter_ can be parsed in Python code into proper types for the params dictionary.

test test_booster_load_params_when_passed_model_str passes

This PR is related to https://github.com/microsoft/LightGBM/issues/2613

A couple of questions:

  • why isn't loaded_parameter_ a parameter of Boosting instead of GBDT already? The other boosting types don't have parameters?
  • Do I understand correctly that c_api.cpp can only deal with gbdt as I see it's hard-coded in https://github.com/microsoft/LightGBM/blob/874e6359241594a7fdfef59924532361c236cf7f/src/c_api.cpp#L109

zyxue avatar Nov 14 '21 17:11 zyxue

@zyxue thanks for coming back to this pull request!

As you work on it, please do not rebase + force push. Use merge commits instead (e.g. git merge master). Overwriting the commit history makes it more difficult for reviewers to understand the changes you've made in response to review comments.

This project squashes all pull request commits into a single commit on merge, so you don't need to worry about having too many commits here.

jameslamb avatar Jan 16 '22 20:01 jameslamb

@jameslamb , gotcha, thank you for the tip.

zyxue avatar Jan 16 '22 22:01 zyxue

Is https://github.com/microsoft/LightGBM/blob/master/include/LightGBM/utils/json11.h expected to be the same as https://raw.githubusercontent.com/dropbox/json11/master/json11.hpp ?

the example on https://github.com/dropbox/json11 doesn't seem to work with the json11 in lightgbm...

zyxue avatar Jan 17 '22 00:01 zyxue

Is https://github.com/microsoft/LightGBM/blob/master/include/LightGBM/utils/json11.h expected to be the same as https://raw.githubusercontent.com/dropbox/json11/master/json11.hpp ?

That is where the json11 code in LightGBM is originally from, but we consider it "vendored in", meaning that since it was first added to this project, LightGBM-specific modifications have sometimes been made to it.

jameslamb avatar Jan 17 '22 00:01 jameslamb

@jameslamb , I've reimplemented LGBM_BoosterGetConfig to return parameters as a json string, please let me know what you think (I'm still relatively new to c++).

zyxue avatar Jan 17 '22 04:01 zyxue

please don't make any changes to the organization until another maintainer like @StrikerRUS, @shiyu1994, or @tongwu-msft comments. Hey @jameslamb , should I update now or wait till more comments come in?

zyxue avatar Jan 22 '22 16:01 zyxue

As I said, please wait until another maintainer offers their opinion. While you wait on that, you can try working through the other suggestions like https://github.com/microsoft/LightGBM/pull/4802#discussion_r786419637.

jameslamb avatar Jan 22 '22 16:01 jameslamb

@shiyu1994 can you take a look for this PR?

guolinke avatar Mar 01 '22 13:03 guolinke

Due to a lack of activity on this pull request, we've decided to move forward with a separate PR for this feature in #5424, so I'm going to close this.

@zyxue thanks very much for your interest in LightGBM and for attempting this! If you have more time to work with us in the future, we'd welcome additional contributions. I'd be happy to suggest some smaller contributions which might not involve as much discussion and require as much of your time and energy.

jameslamb avatar Aug 31 '22 21:08 jameslamb

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[bot] avatar Nov 15 '23 00:11 github-actions[bot]