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] Fix bug of lr updater hook #907

Merged
merged 4 commits into from
Apr 9, 2021

Conversation

zhouzaida
Copy link
Collaborator

@zhouzaida zhouzaida commented Mar 25, 2021

Fix bug of #887

[Next PR]
I will do more unittest in https://github.com/open-mmlab/mmcv/blob/master/tests/test_runner/test_hooks.py.
But, maybe we should refactor the code test_hooks.py to make the test logic clearer.

├── test_basemodule.py
├── test_checkpoint.py
├── test_dist_utils.py
├── test_fp16.py
├── test_hooks.py
├── test_optimizer.py
├── test_runner.py
└── test_utils.py

to

├── test_basemodule.py
├── test_checkpoint.py
├── test_dist_utils.py
├── test_fp16.py
├── test_hooks
│   ├── test_checkpoint_hook.py
│   └── test_lr_updater_hook.py
├── test_hooks.py
├── test_optimizer.py
├── test_runner.py
└── test_utils.py

@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #907 (0ff368e) into master (f0c43fd) will increase coverage by 0.72%.
The diff coverage is 37.31%.

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

@@            Coverage Diff             @@
##           master     #907      +/-   ##
==========================================
+ Coverage   64.62%   65.34%   +0.72%     
==========================================
  Files         148      148              
  Lines        9354     9390      +36     
  Branches     1698     1715      +17     
==========================================
+ Hits         6045     6136      +91     
+ Misses       2962     2914      -48     
+ Partials      347      340       -7     
Flag Coverage Δ
unittests 65.34% <37.31%> (+0.72%) ⬆️

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

Impacted Files Coverage Δ
mmcv/runner/hooks/lr_updater.py 64.38% <0.00%> (+6.83%) ⬆️
mmcv/runner/hooks/momentum_updater.py 60.60% <49.01%> (+9.12%) ⬆️
mmcv/runner/checkpoint.py 69.42% <0.00%> (+1.27%) ⬆️
mmcv/runner/base_runner.py 72.17% <0.00%> (+4.34%) ⬆️
mmcv/runner/hooks/logger/base.py 68.93% <0.00%> (+5.82%) ⬆️
mmcv/runner/hooks/logger/text.py 72.63% <0.00%> (+11.57%) ⬆️

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 f0c43fd...da386ce. Read the comment docs.

@zhouzaida zhouzaida requested a review from ycxioooong March 26, 2021 09:28
@zhouzaida zhouzaida changed the title [Feature] fix bug of lr updater hook [Feature] Fix bug of lr updater hook Mar 29, 2021
@zhouzaida zhouzaida requested a review from nbei March 30, 2021 09:44
@hellock
Copy link
Member

hellock commented Apr 2, 2021

ping @nbei

Copy link
Contributor

@nbei nbei left a comment

Choose a reason for hiding this comment

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

Could this pr contain the unit test for the modified parts?

@zhouzaida zhouzaida changed the title [Feature] Fix bug of lr updater hook [Fix] Fix bug of lr updater hook Apr 6, 2021
@zhouzaida
Copy link
Collaborator Author

zhouzaida commented Apr 6, 2021

Could this pr contain the unit test for the modified parts?

OK

ping @nbei

@zhouzaida zhouzaida requested a review from ZwwWayne April 8, 2021 14:46
@ZwwWayne
Copy link
Collaborator

ZwwWayne commented Apr 9, 2021

Agree to refactor the test_hooks directory. The test_hooks.py can also be moved into test_hooks/ directory.

@zhouzaida
Copy link
Collaborator Author

Agree to refactor the test_hooks directory. The test_hooks.py can also be moved into test_hooks/ directory.

Got it, I will refactor the unit test of hooks in the next PR.

@ZwwWayne ZwwWayne merged commit d525cfd into open-mmlab:master Apr 9, 2021
@zhouzaida zhouzaida deleted the fix_bug_of_lr_updater_hook branch April 9, 2021 02:20
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.

4 participants