-
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
[python] Fix training on subset constructed without params #5213
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. After this change it's probably safe to change this function
LightGBM/python-package/lightgbm/basic.py
Lines 292 to 295 in 2871b53
def param_dict_to_str(data): | |
"""Convert Python dictionary to string, which is passed to C API.""" | |
if data is None or not data: | |
return "" |
None
.
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.
Looks good, thanks!
I left one very very minor suggestion, but marking "approve" so you can merge immediately after addressing it if you agree.
Hmm, are you sure in this? LightGBM/python-package/lightgbm/basic.py Line 2575 in 7d89ab4
there is no guarantee we haven't forgotten to do such None -> {} replacement somewhere. I think it's better to leave that explicit None handling in param_dict_to_str() .
|
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. |
Fixed #5170.
Formally, we change the signature of the public
get_params()
method in this PR: it returns an empty dict instead ofNone
. But I'm not sure about marking this PR a breaking change because previously this method could return empty dicts as well.