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

Change dict update order #1108

Merged
merged 4 commits into from
Jun 29, 2021
Merged

Change dict update order #1108

merged 4 commits into from
Jun 29, 2021

Conversation

antoniolanza1996
Copy link
Contributor

Motivation

This PR solves problem 2 described here: open-mmlab/mmocr#292: if you run training with cfg.resume_from = 'epoch_50.pth', all the checkpoints created during this resumed training (e.g. epoch_51.pth, epoch_52.pth, epoch_53.pth) still continue to have epoch and iter equal to the values in epoch_50.pth.

Modification

With this PR I fixed the dict update order. In particular, meta.update(self.meta) should be done before meta.update(epoch=self.epoch + 1, iter=self.iter) because self.meta contains epoch and iter from resumed checkpoint.

@CLAassistant
Copy link

CLAassistant commented Jun 17, 2021

CLA assistant check
All committers have signed the CLA.

@yeliudev
Copy link
Contributor

yeliudev commented Jun 18, 2021

In downstream repos like mmdetection/mmocr, some other meta information (e.g. old mmdet_version/mmocr_version) from the old checkpoint will still be saved in self.meta, and override the new checkpoints when resuming using newer mmdetection/mmocr.

@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #1108 (9f36da0) into master (004c006) will increase coverage by 0.35%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1108      +/-   ##
==========================================
+ Coverage   67.62%   67.98%   +0.35%     
==========================================
  Files         159      159              
  Lines       10283    10416     +133     
  Branches     1857     1896      +39     
==========================================
+ Hits         6954     7081     +127     
- Misses       2964     2967       +3     
- Partials      365      368       +3     
Flag Coverage Δ
unittests 67.98% <100.00%> (+0.35%) ⬆️

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

Impacted Files Coverage Δ
mmcv/runner/epoch_based_runner.py 83.80% <100.00%> (+1.11%) ⬆️
mmcv/runner/iter_based_runner.py 57.85% <100.00%> (+1.02%) ⬆️
mmcv/runner/hooks/evaluation.py 86.70% <0.00%> (-1.46%) ⬇️
mmcv/utils/parrots_wrapper.py 60.71% <0.00%> (-0.31%) ⬇️
mmcv/cnn/utils/weight_init.py 91.34% <0.00%> (-0.04%) ⬇️
mmcv/image/__init__.py 100.00% <0.00%> (ø)
mmcv/utils/__init__.py 88.23% <0.00%> (ø)
mmcv/image/io.py 88.46% <0.00%> (+0.08%) ⬆️
mmcv/image/geometric.py 94.67% <0.00%> (+0.47%) ⬆️
... and 6 more

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 004c006...9f36da0. Read the comment docs.

@zhouzaida zhouzaida requested a review from innerlee June 18, 2021 08:06
@zhouzaida
Copy link
Collaborator

LGTM

@zhouzaida zhouzaida requested a review from dreamerlin June 24, 2021 05:23
This was referenced Jun 24, 2021
@ZwwWayne ZwwWayne mentioned this pull request Jun 27, 2021
20 tasks
@ZwwWayne
Copy link
Collaborator

LGTM, can be merged after adding comments of explanation.

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.

6 participants