Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix #565 #582

Merged
merged 31 commits into from
Feb 1, 2021
Merged

Fix #565 #582

merged 31 commits into from
Feb 1, 2021

Conversation

kennymckormick
Copy link
Member

No description provided.

HaodongDuan and others added 28 commits October 16, 2020 17:35
@kennymckormick kennymckormick linked an issue Feb 1, 2021 that may be closed by this pull request
@innerlee innerlee changed the title Fix#565 Fix #565 Feb 1, 2021
@@ -432,13 +432,15 @@ def _load_conv_params(conv, state_dict_tv, module_name_tv,
"""

weight_tv_name = module_name_tv + '.weight'
conv.weight.data.copy_(state_dict_tv[weight_tv_name])
loaded_param_names.append(weight_tv_name)
if conv.weight.data.shape == state_dict_tv[weight_tv_name].shape:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to issue some warning if the shapes are mismatched?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think no warning is OK, since:

  1. The shape mismatch may only happen when in_channels != 3. In this case, the shape mismatch is not unexpected. Users assume that pre-train weights are for other modules (except conv1).
  2. There is already a warning message in _load_torchvision_checkpoint, which indicates that parameters of conv1 and bn1 are not loaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its okay if pytorch could warn the user. As long as there are warnings, I'm ok with the change.

@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #582 (abd6165) into master (1d15fb7) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #582      +/-   ##
==========================================
- Coverage   84.17%   84.13%   -0.04%     
==========================================
  Files         121      121              
  Lines        8613     8617       +4     
  Branches     1427     1431       +4     
==========================================
  Hits         7250     7250              
- Misses       1014     1015       +1     
- Partials      349      352       +3     
Flag Coverage Δ
unittests 84.12% <50.00%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
mmaction/models/backbones/resnet.py 93.15% <50.00%> (-1.74%) ⬇️

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 1d15fb7...abd6165. Read the comment docs.

@innerlee
Copy link
Contributor

innerlee commented Feb 1, 2021

Changes are not covered by tests

@kennymckormick
Copy link
Member Author

Changes are not covered by tests

I think it is covered: I add a testing case in which the ResNet is initialized with in_channels=10 and use torchvision://resnet50 for initialization.

@innerlee
Copy link
Contributor

innerlee commented Feb 1, 2021

Then there must be tracking issues in Codcov

@innerlee innerlee removed the need test label Feb 1, 2021
@congee524
Copy link
Contributor

resnet is been tested in tests/test_models/test_common_modules/test_resnet.py.

In our unit test refactor doc, resnet is considered as common module

@kennymckormick
Copy link
Member Author

resnet is been tested in tests/test_models/test_common_modules/test_resnet.py.

In our unit test refactor doc, resnet is considered as common module

Sorry I didn't see it before, I will reorganize related unittests.

@innerlee innerlee merged commit 5c1093e into open-mmlab:master Feb 1, 2021
@kennymckormick kennymckormick deleted the Fix#565 branch March 12, 2021 11:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TSN temporal stream
3 participants