[test schedulers] adjust to test the first step's reading #6429
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As I was working on a new scheduler, it was difficult to match numbers since the first step's reading was dropped in
unwrap_schedule
wrappers (they were taking the measurement after stepping). This PR adjusts the wrappers to first take a reading and then step.This PR also makes a small refactoring to move all the unwrapping into the script, so the test just compares 2 lists. (avoiding multiple
[l[0] for l in lrs_1]
)The updated table is:
Unrelated to the changes suggestion in this PR, it exposes 2 minor issues:
To illustrate, see this change in reported number for
get_polynomial_decay_schedule_with_warmup
:the expected last step of
1e-07
is not there. It never was.0.0
in all schedulers, except inget_constant_schedule
, so the first step does nothing. This can be fixed with a potentially addedmin_lr=1e-7
to all schedulers, as it was suggested by @sshleifer in one of the recent scheduler-related PRs.Let me know if this better fits into its own issue, as these issues have nothing to do with the PR itself. Or perhaps the 2 issues are just unimportant...