Skip to content

Enhance ShufflenetV2 #892

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

Merged
merged 2 commits into from
May 8, 2019
Merged

Enhance ShufflenetV2 #892

merged 2 commits into from
May 8, 2019

Conversation

barrh
Copy link
Contributor

@barrh barrh commented May 7, 2019

Class shufflenetv2 receives stages_repeats and stages_out_channels arguments.

Following discussion in: #889

@barrh barrh force-pushed the shufflenet_multipliers branch from e3b6a7c to d91e35f Compare May 7, 2019 14:46
@barrh barrh changed the title Enhange ShufflenetV2 Enhance ShufflenetV2 May 7, 2019
Copy link
Contributor

@ekagra-ranjan ekagra-ranjan left a comment

Choose a reason for hiding this comment

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

Both #889 and #892 address different issues as discussed in #889 although the following changes are needed to remove the getpaperParam function completely.

@@ -165,16 +161,16 @@ def shufflenetv2(pretrained=False, num_classes=1000, width_mult=1, **kwargs):


def shufflenetv2_x0_5(pretrained=False, num_classes=1000, **kwargs):
return shufflenetv2(pretrained, num_classes, 0.5)
return _shufflenetv2(pretrained, num_classes, '0.5')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pass the stage_repeats argument as a list of int as suggested by @fmassa in this comment? Doing so would remove the need of the function getpaperParam completely when both #889 and #892 are merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be nice to expose to the user paper's parameters, either by staticmethod, stand-alone function, or documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether it would be better. @fmassa could guide you on this. But the above requested changes will make the implementation similar to that of ResNets.

Copy link
Member

Choose a reason for hiding this comment

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

I think that I prefer the approach that is followed by resnet

return _resnet('resnet101', Bottleneck, [3, 4, 23, 3], pretrained, progress,

Paper parameters are specified in the functions (and defines the meaning of 0_5 / 1_0 etc), but the base class should be agnostic to it if possible.

@fmassa
Copy link
Member

fmassa commented May 7, 2019

@barrh this overlaps a bit with #889 . I'll be merging #889 as it addresses all the points I had, can you then rebase on top of master once it's merged?

@codecov-io
Copy link

codecov-io commented May 7, 2019

Codecov Report

Merging #892 into master will decrease coverage by 0.24%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #892      +/-   ##
=========================================
- Coverage   56.95%   56.7%   -0.25%     
=========================================
  Files          43      43              
  Lines        3559    3562       +3     
  Branches      544     546       +2     
=========================================
- Hits         2027    2020       -7     
- Misses       1414    1420       +6     
- Partials      118     122       +4
Impacted Files Coverage Δ
torchvision/models/shufflenetv2.py 86.04% <73.33%> (-4.32%) ⬇️
torchvision/transforms/transforms.py 81.25% <0%> (-1.3%) ⬇️

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 dc3ac29...93ab83e. Read the comment docs.

@barrh
Copy link
Contributor Author

barrh commented May 7, 2019

@fmassa sure.

barrh added 2 commits May 7, 2019 20:36
Class shufflenetv2 receives `stages_repeats` and `stages_out_channels` arguments.
@barrh barrh force-pushed the shufflenet_multipliers branch from d91e35f to 69c3a1f Compare May 7, 2019 18:42
@barrh
Copy link
Contributor Author

barrh commented May 7, 2019

rebased. I didn't test it because python setup.py install behaves funny following the c++ patch.

@fmassa
Copy link
Member

fmassa commented May 7, 2019

@barrh for easy developing Python code which contains C++ extensions, I generally do python setup.py build develop, which installs locally, so that the python changes are applied immediately and there is no need to recompile

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@fmassa fmassa merged commit 43ab2fe into pytorch:master May 8, 2019
@barrh barrh deleted the shufflenet_multipliers branch May 8, 2019 11:15
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