-
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] make log_evaluation
callback pickleable
#5101
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.
Looks great! Thanks very much for starting work on this issue, I think it'll be great to have better compatibility with frameworks like Ray and Dask.
I just left two minor comments.
def __init__(self, period: int = 1, show_stdv: bool = True) -> None: | ||
self.order = 10 | ||
self.before_iteration = 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.
very very minor comment, but could this empty line be removed, to group all assignments to self.*
properties?
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.
I believe it would be better for code readability to split properties that are common for all callbacks (order
and before_iteration
) and specific properties for a particular callback. WDYT?
Refer to
LightGBM/python-package/lightgbm/callback.py
Lines 204 to 210 in 417c732
self.order = 30 | |
self.before_iteration = False | |
self.stopping_rounds = stopping_rounds | |
self.first_metric_only = first_metric_only | |
self.verbose = verbose | |
self.min_delta = min_delta |
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.
ah I see, that makes sense. Ok yeah I'm fine with that too! I don't feel strongly either way.
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. |
Contributes to #5080.
In
master
,test_log_evaluation_callback_is_picklable()
test fails with