Skip to content

Refactoring and moving MobileNetV2 to make it reusable #3177

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 5 commits into from
Dec 17, 2020

Conversation

datumbox
Copy link
Contributor

@datumbox datumbox commented Dec 16, 2020

This PR works on the prerequisites for supporting other MobileNet models.

Changes:

  • Moving the implementation of MobileNetV2 from mobilenet.py to mobilenetv2.py in a BC way.
  • Making ConvBNReLU class to be reusable and support more activation functions.

It's going to be easier to review the PR by checking individual commits.

@datumbox datumbox changed the title [WIP] Refactoring and moving MobileNetV2 to make it reusable Refactoring and moving MobileNetV2 to make it reusable Dec 16, 2020
@datumbox datumbox requested a review from fmassa December 16, 2020 13:22
@datumbox
Copy link
Contributor Author

datumbox commented Dec 16, 2020

@fmassa I tried doing the changes incrementally so that we can keep the git blame history but git is acting up when we squash. I can try splitting this PR into two:

The above could also be achieved by restructuring the commits and using "merge commit" instead of "squash and merge". Unfortunately, no matter how we do it, it has the side-effect that leaves temporarily the master broken for 1 commit. Thus we will need to squash the two PRs when we sync FBcode. Despite the hassle I prefer this approach from merging this version as is; I think maintaining the history is beneficial. Thoughts?

@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #3177 (db7522b) into master (91e03b9) will increase coverage by 0.00%.
The diff coverage is 85.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3177   +/-   ##
=======================================
  Coverage   73.48%   73.49%           
=======================================
  Files          99      101    +2     
  Lines        9230     9235    +5     
  Branches     1476     1477    +1     
=======================================
+ Hits         6783     6787    +4     
  Misses       1991     1991           
- Partials      456      457    +1     
Impacted Files Coverage Δ
torchvision/models/mobilenetv2.py 83.14% <83.14%> (ø)
torchvision/models/quantization/mobilenetv2.py 87.75% <87.75%> (ø)
torchvision/models/mobilenet.py 100.00% <100.00%> (+16.27%) ⬆️
torchvision/models/quantization/mobilenet.py 100.00% <100.00%> (+12.24%) ⬆️

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 91e03b9...cead9be. Read the comment docs.

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.

LGTM, thanks!

@fmassa
Copy link
Member

fmassa commented Dec 17, 2020

Let's merge this PR as is, git blame won't be in an ideal state but we can always go back in time with a bit more effort.

@datumbox datumbox merged commit 4cbe714 into pytorch:master Dec 17, 2020
@datumbox datumbox deleted the refactoring/mobilenetv2_bc_move branch December 17, 2020 09:46
facebook-github-bot pushed a commit that referenced this pull request Dec 23, 2020
Summary:
* Moving mobilenet.py to mobilenetv2.py

* Adding mobilenet.py for BC.

* Extending ConvBNReLU for reuse.

* Reduce import scope on mobilenet to only the public and versioned classes and methods.

Reviewed By: fmassa

Differential Revision: D25679211

fbshipit-source-id: 72d8eadeef42a93879bbe4a61b6611023db29669
@datumbox datumbox mentioned this pull request Jan 5, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants