-
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-package] clarify max_depth warning and limit when it is emitted #6402
Conversation
@@ -1134,7 +1134,7 @@ struct Config { | |||
static const std::string DumpAliases(); | |||
|
|||
private: | |||
void CheckParamConflict(); | |||
void CheckParamConflict(const std::unordered_map<std::string, std::string>& params); |
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.
params
here contains the content of what was passed through by the used (with aliases already resolved). So it can be used to differentiate between "user code passed num_leaves=31
" and "num_leaves=31
because that's the default and user didn't pass any value for it".
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.
Makes sense to me! A few small comments... 😄
Co-authored-by: Oliver Borchert <[email protected]>
I'll review this tomorrow. Thanks. |
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! (modulo linting CI job 😄)
@shiyu1994 could you please review this this week? |
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 to me. Thank you for the change.
fixes #2898
fixes #5734
Modifies a warning that has historically been confusing for some LightGBM users. @shiyu1994 explained it very well in #2898 (comment):
This PR proposes an implementation of @shiyu1994 's proposal from further down in that comment.
As a side effect of this change, that also means the warning in question will never be raised from the
scikit-learn
estimators. They always passnum_leaves
in params, from this keyword argument:LightGBM/python-package/lightgbm/sklearn.py
Line 463 in 28536a0
I think that's ok. We have the docs in https://lightgbm.readthedocs.io/en/latest/Parameters-Tuning.html#tune-parameters-for-the-leaf-wise-best-first-tree. I'd rather have the
scikit-learn
estimators not emit this warning than take on the added complexity that'd be required to detect whether or notnum_leaves
was explicitly provided in the constructor keyword args.Notes for Reviewers
I would really like a review from @shiyu1994, to be sure I've interpreted #2898 (comment) correctly.
Would also appreciate feedback from any of the people involved in the previous discussions about this one whether this new warning is clearer.
cc @bfrobin446 @AlbertoEAF @dxyzx0 @aEgoist @memeplex @Wang-Yu-Qing @Cat2Li