-
Notifications
You must be signed in to change notification settings - Fork 45
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
CGLS: deprecate tolerance #1892
CGLS: deprecate tolerance #1892
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.
Just a small suggestion on the comments.
Please also create a new issue akin to #1443 linking the code, stating which version it was marked as deprecated in and suggesting removal for v25 (or 26?)
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.
Just a few minor comments and questions, but generally looks good to me.
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.
All looks good to me.
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 for addressing the comments and in particular splitting to two separate tolerances and adding the explanation of historical CIL usage and original source. Approving with a few very minor suggestions.
class EarlyStoppingObjectiveValue(Callback): | ||
'''Callback that stops iterations if the change in the objective value is less than a provided threshold value. | ||
|
||
Parameters | ||
---------- | ||
threshold: float, default 1e-6 | ||
|
||
Note | ||
----- | ||
This callback only compares the last two calculated objective values. If `update_objective_interval` is greater than 1, the objective value is not calculated at each iteration (which is the default behaviour), only every `update_objective_interval` iterations. | ||
|
||
''' | ||
def __init__(self, threshold=1e-6): | ||
self.threshold=threshold |
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 strongly disapprove of this. Far superior naming, vis. https://keras.io/api/callbacks/early_stopping/:
EarlyStopping
,EarlyStoppingObjectiveDelta
,EarlyStoppingObjectiveDelta
,EarlyStoppingObjectiveChange
delta
,change
Changes
tolerance
as a kwarg here to keep backwards compatibility if users have a custom tolerance with a plan to deprecate this fully in a later version.Testing you performed
Related issues/links
Closes #1891, closes #1894
Checklist
Contribution Notes
Please read and adhere to the developer guide and local patterns and conventions.