Skip to content
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

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

githubnemo
Copy link
Collaborator

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.

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.
Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "intertolerable"?

Copy link
Collaborator Author

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 :)

Copy link
Collaborator

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.

Comment on lines 49 to 51
# O I I I I epoch classification
# 0 1 2 3 4 number of bad epochs
# * * * epochs with LR reduction
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be:

Suggested change
# 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

Copy link
Collaborator Author

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.

Comment on lines 53 to 55
# 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])
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

@@ -87,6 +87,11 @@ def simulate(self, steps, initial_lr):
initial_lr: float
Initial learning rate

step_args: Any
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Changed!

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``.
Copy link
Collaborator

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?

Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a 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.

@githubnemo githubnemo merged commit f239d8a into skorch-dev:master Jan 9, 2025
16 checks passed
githubnemo added a commit that referenced this pull request Jan 9, 2025
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants