Adds Cyclical Learning Rate and Momentum#18001
Adds Cyclical Learning Rate and Momentum#18001sampepose wants to merge 1 commit intopytorch:masterfrom
Conversation
|
thanks Sam (and @thomasjpfan too). Let me know if you need a review / re-review. |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@sampepose has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
b4fdfbf to
3deec2c
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@sampepose has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Hey @soumith! I'd appreciate a review, thank you! |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@sampepose has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
ping @soumith :) |
soumith
left a comment
There was a problem hiding this comment.
comments are minor, but need necessary changes.
Thanks, the PR looks pretty good.
torch/optim/lr_scheduler.py
Outdated
There was a problem hiding this comment.
I'd suggest to not have a default for the base learning rate. It makes it too easy to shoot oneself in the foot
torch/optim/lr_scheduler.py
Outdated
There was a problem hiding this comment.
You either want to make the max_lr default be a multiple of base_lr, or simply remove a default
torch/optim/lr_scheduler.py
Outdated
There was a problem hiding this comment.
" mode parameter is ignored" -> "If specified, then mode is ignored"
torch/optim/lr_scheduler.py
Outdated
There was a problem hiding this comment.
what exactly does -1 do here?
There was a problem hiding this comment.
also, it's not clear what this is used for, explaining what the index of the last batch is, or where / why it is used will help
torch/optim/lr_scheduler.py
Outdated
There was a problem hiding this comment.
Shouldn't it be:
train_batch(...)
scheduler.step()
torch/optim/lr_scheduler.py
Outdated
There was a problem hiding this comment.
comments above about default values
torch/optim/lr_scheduler.py
Outdated
There was a problem hiding this comment.
do you mean step size_down = step_size_down if step_size_down is not None else step_size_up
torch/optim/lr_scheduler.py
Outdated
There was a problem hiding this comment.
it seems safer to actually convert step_size_up and step_size_down to float immediately on entry into the function.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@sampepose has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
5d1a730 to
00fa02c
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
@sampepose has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@sampepose has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@sampepose merged this pull request in 8635078. |
This implements a cyclical learning rate (CLR) schedule with an optional inverse cyclical momentum. More info about CLR: https://github.com/bckenstler/CLR
This is finishing what #2016 started. Resolves #1909.