Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix warmup lr #1079

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix warmup lr #1079

wants to merge 1 commit into from

Conversation

twmht
Copy link
Contributor

@twmht twmht commented Jun 5, 2021

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

Fix bug when warmup by epoch.

Modification

When using the following lr_config, this is how ranger works.

lr_config={
type='CosineAnnealing',
warmup='constant',
warmup_iters=75,
warmup_by_epoch=True
min_lr=0,
by_epoch=True,
warmup_ration=1
}

total_epochs=100

in the current implementations, it does not work as expected. it would decay the learning rate by cosine in second epoch.

this PR would fix the bug.

@codecov
Copy link

codecov bot commented Jun 5, 2021

Codecov Report

Merging #1079 (c0190a8) into master (d212bd5) will increase coverage by 0.00%.
The diff coverage is 57.14%.

❗ Current head c0190a8 differs from pull request most recent head 65d0500. Consider uploading reports for the commit 65d0500 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1079   +/-   ##
=======================================
  Coverage   65.72%   65.72%           
=======================================
  Files         157      157           
  Lines       10092    10098    +6     
  Branches     1828     1830    +2     
=======================================
+ Hits         6633     6637    +4     
- Misses       3113     3115    +2     
  Partials      346      346           
Flag Coverage Δ
unittests 65.72% <57.14%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmcv/runner/hooks/lr_updater.py 70.03% <57.14%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d212bd5...65d0500. Read the comment docs.

@wangg12
Copy link
Contributor

wangg12 commented Jun 7, 2021

The lr schedule for ranger could be achieved by #1066

@wangg12
Copy link
Contributor

wangg12 commented Jun 7, 2021

I think the lr functions in mmcv usually treat the beginning iteration as 1 (the first iteration) regardless of warmup. If you change it to the end of warmup iter, it might break the behavior of many other models.

@twmht
Copy link
Contributor Author

twmht commented Jun 9, 2021

@wangg12

What I think is that this is a bug-fix-PR, not a feature enhancement PR.

@wangg12
Copy link
Contributor

wangg12 commented Jun 9, 2021

Not sure if it is a "bug fix", because it breaks the old behavior.

@twmht
Copy link
Contributor Author

twmht commented Jun 10, 2021

@wangg12

Can you explain an example which is broken by this PR?

@hellock hellock requested a review from dreamerlin June 15, 2021 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants