-
Notifications
You must be signed in to change notification settings - Fork 395
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
Add step_args parameter to LRScheduler.simulate #1077
Conversation
The purpose of this change is to allow for simulation of scheduling policies such as `ReduceLROnPlateau` to be simulated properly. If `step_args` is an indexable object, it is indexed using the current simulated epoch to get closer to real-life behavior, e.g. simulating a real loss curve and the corresponding behavior of the LR scheduler policy.
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 the PR to enhance the capabilities of simulate
. My comments are mostly about clarifications.
Failing tests require #1078 to be merged.
steps=5, initial_lr=1, step_args=0.5 | ||
) | ||
# O = OK epoch | ||
# I = intertolerable epoch |
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.
What is "intertolerable"?
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.
This is terminology taken from the docs
In the second epoch, if the performance is worse than the baseline, we have what is considered an intolerable epoch.
Drastic terminology but I wanted to adhere to already defined terms :)
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.
Drastic indeed, but it's in the Shakespeare corpus, so it must be a common English word.
# O I I I I epoch classification | ||
# 0 1 2 3 4 number of bad epochs | ||
# * * * epochs with LR reduction |
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.
Should it be:
# O I I I I epoch classification | |
# 0 1 2 3 4 number of bad epochs | |
# * * * epochs with LR reduction | |
# O I I I I epoch classification | |
# 0 1 2 3 4 number of bad epochs | |
# * epochs with LR reduction |
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.
No, it's actually
# O I I I I epoch classification
# 0 1 2 1 2 number of bad epochs
# * * epochs with LR reduction
I've updated the comment but the gist is that the bad epoch counter is reset after an LR reduction.
# note that simulate returns the LRs before the step, not after, | ||
# so we're seeing only 4 updated values. | ||
assert all(lrs == [1] + [1, 1, 0.1, 0.1]) |
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.
Do we see "4 updated values"? Why use [1] + ...
here but not in the next test?
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've removed the [1]
, it did not help visualize what's going on. I've also changed it to "4 simulated values" since the first value to return is always the init. LR.
skorch/callbacks/lr_scheduler.py
Outdated
@@ -87,6 +87,11 @@ def simulate(self, steps, initial_lr): | |||
initial_lr: float | |||
Initial learning rate | |||
|
|||
step_args: Any |
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.
Would it be more helpful to change the type to None or float or list of float (default=None)
?
Yes, technically it doesn't have to be a list but I don't think it still makes it easier for users to know what they need to pass.
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.
Agreed. Changed!
skorch/callbacks/lr_scheduler.py
Outdated
step_args: Any | ||
Argument to the `.step()` function of the policy. If it is an | ||
indexable object the simulation will try to associate every step of | ||
the simulation with an entry in ``step_args``. |
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.
Also describe what happens when None
or float
are passed?
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 the changes, it's now much easier to understand what happens.
Please welcome skorch 1.1.0 - a smaller release with a few fixes, a new notebook showcasing learning rate schedulers and mainly support for scikit-learn 1.6.0. Full list of changes: ### Added - Added a [notebook](https://github.com/skorch-dev/skorch/blob/master/notebooks/Learning_Rate_Scheduler.ipynb) that shows how to use Learning Rate Scheduler in skorch.(#1074) ### Changed - All neural net classes now inherit from sklearn's [`BaseEstimator`](https://scikit-learn.org/stable/modules/generated/sklearn.base.BaseEstimator.html). This is to support compatibility with sklearn 1.6.0 and above. Classification models additionally inherit from [`ClassifierMixin`](https://scikit-learn.org/stable/modules/generated/sklearn.base.ClassifierMixin.html) and regressors from [`RegressorMixin`](https://scikit-learn.org/stable/modules/generated/sklearn.base.RegressorMixin.html). (#1078) - When using the `ReduceLROnPlateau` learning rate scheduler, we now record the learning rate in the net history (`net.history[:, 'event_lr']` by default). It is now also possible to to step per batch, not only by epoch (#1075) - The learning rate scheduler `.simulate()` method now supports adding step args which is useful when simulation policies such as `ReduceLROnPlateau` which expect metrics to base their schedule on. (#1077) - Removed deprecated `skorch.callbacks.scoring.cache_net_infer` (#1088) ### Fixed - Fix an issue with using `NeuralNetBinaryClassifier` with `torch.compile` (#1058)
The purpose of this change is to allow for simulation of scheduling policies such as
ReduceLROnPlateau
to be simulated properly.If
step_args
is an indexable object, it is indexed using the current simulated epoch to get closer to real-life behavior, e.g. simulating a real loss curve and the corresponding behavior of the LR scheduler policy.