LightGBM

LightGBM copied to clipboard

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 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 , gotcha, thank you for the tip.

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...

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 , 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++).

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?

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.

@shiyu1994 can you take a look for this PR?

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.

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.