Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Fix lr scheduler tests & lr schedulers #4242

Merged
merged 4 commits into from
Dec 12, 2021
Merged

Fix lr scheduler tests & lr schedulers #4242

merged 4 commits into from
Dec 12, 2021

Conversation

DrMatters
Copy link
Contributor

Patch description
This patch fixes a wrong parameter name in LR schedulers test

Testing steps
This is a fix for tests

Other information
Seems, like warmup implementation for all schedulers has never been tested. This was because the testing code was unreachable due to the wrong parameter name ('warump_updates'). After fixing the parameter of the test, some tests started to fail.

@DrMatters DrMatters changed the title Fix a typo in parameter name in LR schedulers test Fix lr scheduler tests & lr schedulers Dec 12, 2021
@DrMatters
Copy link
Contributor Author

2021-12-12_14-25-26
Have you noticed before that the warmup is not finished properly? The target LR for this plot is 7e-6, but it only reaches 6.93e-6 due to warmup discontinuing one step earlier

@DrMatters
Copy link
Contributor Author

I've fixed lr_schedulers by updating def _is_lr_warming_up (so they pass tests), but it is a kludge meant to help to identify the root cause of the incorrect behavior. Using the kludge, you can see and verify that the problem is coupled with the moment of switching the warmup scheduler to the main scheduler.

Copy link
Contributor

@stephenroller stephenroller left a comment

Choose a reason for hiding this comment

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

Awesome, this is great. Thank you so much.

I'm going to smash land, and then if you just rebase your other PR, I'll be happy to accept it.

Thanks so much for your hard work!!!

@stephenroller stephenroller merged commit 359cfe3 into facebookresearch:main Dec 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants