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

[Feature] Enable torchvision backbones #720

Merged
merged 42 commits into from
Mar 25, 2021

Conversation

kennymckormick
Copy link
Member

An example:
backbone=dict(type='torchvision.resnet50')

HaodongDuan and others added 30 commits October 16, 2020 17:35
@innerlee innerlee requested a review from dreamerlin March 13, 2021 10:25
@@ -69,7 +85,8 @@ def __init__(self,

def init_weights(self):
"""Initialize the model network weights."""
self.backbone.init_weights()
if self.backbone_from in ['mmcls', 'mmaction2']:
self.backbone.init_weights()
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened for torchvision

Copy link
Member Author

Choose a reason for hiding this comment

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

torchvision init all modules in the init function

Copy link
Contributor

Choose a reason for hiding this comment

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

Users might call this function by hand

Copy link
Member Author

Choose a reason for hiding this comment

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

for which case?

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean users want to init the backbones in their own way?

Copy link
Contributor

Choose a reason for hiding this comment

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

No specific case. I'm concerning the semantic of this function. If the function name is "init weights", then it is expected to init weights. However, current code can silently refuse to init weights based on the value of some external variables.

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 can add a warning in init_weights of recognizer2d

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this for now. If this is not correct, there will be bug reports eventually.

if self.backbone_from == 'torchvision':
if len(x.shape) == 4 and (x.shape[2] > 1 or x.shape[3] > 1):
# apply adaptive avg pooling
x = nn.AdaptiveAvgPool2d(1)(x)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is needed

Copy link
Member Author

Choose a reason for hiding this comment

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

The shapes of different torchvision models' outputs vary, it may be N x C x H x W, N x C x 1 x 1 or N x C.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if the special-cased processing is expected. Why backbones from other sources are not handled?

Copy link
Member Author

Choose a reason for hiding this comment

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

Backbones from other sources are written by us (mmcls / mmaction2), thus are more standard. In torchvision, there can be some legacy issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay

@codecov
Copy link

codecov bot commented Mar 13, 2021

Codecov Report

Merging #720 (d6fc22f) into master (8a79e52) will decrease coverage by 0.03%.
The diff coverage is 75.00%.

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

@@            Coverage Diff             @@
##           master     #720      +/-   ##
==========================================
- Coverage   85.45%   85.41%   -0.04%     
==========================================
  Files         130      130              
  Lines        9371     9401      +30     
  Branches     1572     1580       +8     
==========================================
+ Hits         8008     8030      +22     
- Misses        962      967       +5     
- Partials      401      404       +3     
Flag Coverage Δ
unittests 85.40% <75.00%> (-0.04%) ⬇️

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

Impacted Files Coverage Δ
mmaction/models/recognizers/recognizer2d.py 88.75% <63.63%> (-4.01%) ⬇️
mmaction/models/recognizers/base.py 68.38% <80.95%> (+1.71%) ⬆️

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 8a79e52...1ab9add. Read the comment docs.

@dreamerlin dreamerlin mentioned this pull request Mar 15, 2021
10 tasks
@kennymckormick
Copy link
Member Author

Kindly ping @innerlee

@innerlee
Copy link
Contributor

Good to go after rebasing.

@dreamerlin
Copy link
Collaborator

@kennymckormick use pre-commit run --all-files to correct the lint failure.

@kennymckormick
Copy link
Member Author

@innerlee ready for merging

@innerlee
Copy link
Contributor

image

The CI error seems real. I guess the model forwarding exceeds memory/cpu quota. Please replace the torchvision.densenet161 with the tiniest possible model.

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.

3 participants