-
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] reset storages in early stopping callback after finishing training #4868
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.
Thank you for working on this. Just leave a comment for now since I'm not sure the inited
is reset correctly.
if env.iteration == env.end_iteration - 1: | ||
if verbose: | ||
best_score_str = '\t'.join([_format_eval_result(x) for x in best_score_list[i]]) | ||
_log_info('Did not meet early stopping. ' | ||
f'Best iteration is:\n[{best_iter[i] + 1}]\t{best_score_str}') | ||
if first_metric_only: | ||
_log_info(f"Evaluated only: {eval_name_splitted[-1]}") | ||
inited = False |
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.
It seems that inited
is reset to False
only when early stopping is not triggered? Because
inited
is reset toFalse
only whenenv.iteration == env.end_iteration - 1
_final_iteration_check
is not called when early stopping is triggered according to
LightGBM/python-package/lightgbm/callback.py
Lines 306 to 327 in 00f87c5
for i in range(len(env.evaluation_result_list)): score = env.evaluation_result_list[i][2] if best_score_list[i] is None or cmp_op[i](score, best_score[i]): best_score[i] = score best_iter[i] = env.iteration best_score_list[i] = env.evaluation_result_list # split is needed for "<dataset type> <metric>" case (e.g. "train l1") eval_name_splitted = env.evaluation_result_list[i][1].split(" ") if first_metric_only and first_metric != eval_name_splitted[-1]: continue # use only the first metric for early stopping if ((env.evaluation_result_list[i][0] == "cv_agg" and eval_name_splitted[0] == "train" or env.evaluation_result_list[i][0] == env.model._train_data_name)): _final_iteration_check(env, eval_name_splitted, i) continue # train data for lgb.cv or sklearn wrapper (underlying lgb.train) elif env.iteration - best_iter[i] >= stopping_rounds: if verbose: eval_result_str = '\t'.join([_format_eval_result(x) for x in best_score_list[i]]) _log_info(f"Early stopping, best iteration is:\n[{best_iter[i] + 1}]\t{eval_result_str}") if first_metric_only: _log_info(f"Evaluated only: {eval_name_splitted[-1]}") raise EarlyStopException(best_iter[i], best_score_list[i]) _final_iteration_check(env, eval_name_splitted, i)
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.
@shiyu1994 Thanks for your review!
It seems that
inited
is reset toFalse
only when early stopping is not triggered?
I believe inited
is reset to False
when early stopping is triggered here
LightGBM/python-package/lightgbm/callback.py
Lines 339 to 340 in dff622c
inited = False | |
raise EarlyStopException(best_iter[i], best_score_list[i]) |
Triggering early stopping means to raise
EarlyStopException
and inited
is reset to False
right before that line.
Also, if it is not reset, the test test_grid_search()
will fail because we use constant custom evaluation metric there which force early stopping to happen at stopping_rounds
iteration because there is no improvement after the first iteration.
@StrikerRUS Thanks for the explanation. Sorry that I did not notice line 339. |
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.
The changes LGTM. Thank you.
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.
thanks very much for the explanation! I think this fix makes sense.
is this going to be merged to master? having a working grid search and other related packaged like OPTUNA work with light GBM is a must for many people and without this update they still don't work properly with light GBM!! from OPTUNA #3145 "LightGBM 3.3.2, which has been released yesterday, does not include the change. We need to wait for the fix..." and recently OPTUNA #3625 |
This PR was merged on December 9, 2021. I think you mean "when will this be released to package managed like PyPI". Due to a lack of maintainer activity, it will probably still be several months until the next release of LightGBM. You can subscribe to #5153 for updates on the next release, and even comment on the linked issues if there are any you'd like to contribute, to move the project closer to that release. |
ahh yes thank you for clarifying this! I installed it on my test notebook and it seems to run fine but I get an error with OPTUNA which I mentioned to them. I installed using the following in case anyone else needs this
|
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. |
Right now
early_stopping()
callback cannot be used with scikit-learn'sGridSearchCV
tool due to that storages for metric results are not cleaned after early stopping occurs. This results in comparing current results with the best iteration for the first grid iteration for the first fold.This was spotted by our
test_grid_search
testLightGBM/tests/python_package_test/test_sklearn.py
Line 276 in fe535a0
which fails with early stopping setup passed via callback with the following error
This PR proposes resetting all global variables in the
_init()
helper function and determining whether initialization is required by the special variableinited
instead of checkingcmp_op
global variable for emptiness, which results right now in that_init()
is called only once during the whole grid search routine.Extracted from #4846.