To change WarmUp Scheduler with ConstantLR and LinearLR#64395
To change WarmUp Scheduler with ConstantLR and LinearLR#64395iramazanli wants to merge 1 commit intopytorch:masterfrom
Conversation
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 93bb484 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
91dd0e0 to
09b1b56
Compare
09b1b56 to
8b18dc0
Compare
8b18dc0 to
37851bb
Compare
Codecov Report
@@ Coverage Diff @@
## master #64395 +/- ##
==========================================
+ Coverage 66.63% 66.66% +0.03%
==========================================
Files 705 707 +2
Lines 92193 92535 +342
==========================================
+ Hits 61432 61690 +258
- Misses 30761 30845 +84 |
37851bb to
f72fe20
Compare
datumbox
left a comment
There was a problem hiding this comment.
@iramazanli Thanks a lot for the fast PR.
I left a few minor comments for which I would like your input, but nothing major.
I also noticed that we don't have any tests for LinearLR that use the new end parameter. We need to adapt the tests to include it I think. Other than this it looks good.
ad8ef81 to
20b5fdb
Compare
datumbox
left a comment
There was a problem hiding this comment.
LGTM, thanks for the speedy changes.
20b5fdb to
05129c9
Compare
|
@iramazanli has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
05129c9 to
aa05f17
Compare
aa05f17 to
93bb484
Compare
|
@iramazanli has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
fmassa
left a comment
There was a problem hiding this comment.
Thanks a lot for the PR Ilqar!
I've made a couple of minor comments for my understanding, otherwise everything LGTM!
| if (self.last_epoch > self.warmup_iters or | ||
| (self.warmup_method == "constant" and self.last_epoch != self.warmup_iters)): | ||
| if (self.last_epoch > self.total_iters): | ||
| return [group['lr'] for group in self.optimizer.param_groups] |
There was a problem hiding this comment.
Don't we need to multiply by end_factor now that we might have an end_factor which is different than 1?
There was a problem hiding this comment.
Actually as this is the recursive definition and at the epoch number total_iters we already have multiplication with end_factor, we no more need to mention multiplication here.
| return [group['lr'] * (1. + (self.end_factor - self.start_factor) / | ||
| (self.total_iters * self.start_factor + (self.last_epoch - 1) * (self.end_factor - self.start_factor))) |
There was a problem hiding this comment.
Just asking to make sure this case is handled properly: can we have end_factor < start_factor (i.e., for a linear decay of the learning rate)?
There was a problem hiding this comment.
yes it can have :)
|
@iramazanli has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@iramazanli merged this pull request in f767cf6. |
Partially unblocks pytorch/vision#4281
Previously we have added WarmUp Schedulers to PyTorch Core in the PR : #60836 which had two mode of execution - linear and constant depending on warming up function.
In this PR we are changing this interface to more direct form, as separating linear and constant modes to separate Schedulers. In particular
will look like
correspondingly.