Update learning rate on each backward pass instead of each forward pass.

[rmrao]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1476.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[codecov]

Codecov Report

Merging #1477 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1477   +/-   ##
======================================
  Coverage      90%     90%           
======================================
  Files          68      68           
  Lines        3804    3805    +1     
======================================
+ Hits         3441    3442    +1     
  Misses        363     363           

[justusschock]

LGTM

[williamFalcon]

I don't understand...

Currently, update_learning_rates is called AFTER .backward, is it not?

The call to:
run_training_batch
does a forward AND backward pass.

And the order is as follows:

run_training_batch
update_learning_rates

[rmrao]

I believe at the moment that run_training_batch runs a forward pass, but only runs a backwards pass if self.batch_idx + 1 % self.accumulate_grac_batches == 0 (see here). This is consistent with how the global step is incremented in run_training_epoch (here).

I didn't move the learning rate update line - just added a condition over it. It might be better / cleaner to move it somewhere else, or to try and move it inside of run_training_batch. Doing that would require rewriting how self.update_learning_rates works, however.

[williamFalcon]

ok got it. that makes sense. it does seem a little dirty at the moment.
Mind adding a todo comment on top of your condition so we know to look into this later?

[mergify]

This pull request is now in conflict... :(

[pep8speaks]

Hello @rmrao! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-20 11:04:10 UTC

[rmrao]

Done - added a TODO to potentially merge optimizer step, lr update, and global step increment.

[mergify]

This pull request is now in conflict... :(