Skip to content

[DONOTMERGE] deform_conv2d with namespaces#2998

Closed
datumbox wants to merge 11 commits intopytorch:masterfrom
datumbox:refactoring/namespaces_ops
Closed

[DONOTMERGE] deform_conv2d with namespaces#2998
datumbox wants to merge 11 commits intopytorch:masterfrom
datumbox:refactoring/namespaces_ops

Conversation

@datumbox
Copy link
Copy Markdown
Contributor

@datumbox datumbox commented Nov 12, 2020 •
edited
Loading

Proof of concept for #2956

  • Renaming C++ files & methods according to recommended naming conventions and aligning them with Python's API.
  • Adding all internal functions in anonymous namespaces and removing their "C-style" static keywords.
  • Syncing, where possible, the names of functions across devices and removing "cpu"/"gpu"/"cuda" from their names.
  • Syncing variable names between the cpp files and their header files.
  • Moving C++ code from header files to cpp files. Each cpp file now has a separate header file with "public" functions.
  • Removing unnecessary repeated includes.
  • Using a similar naming convention as in PyTorch for the kernel implementations in C++ and CUDA.
  • Removing the _param postfix from the variables (DeformConv2d specific detail).

cc @fmassa @mthrok

datumbox
datumbox commented Nov 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

I left some comments to give context to the changes.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 13, 2020 •
edited
Loading

Codecov Report

Merging #2998 (aefe934) into master (b8b08ac) will increase coverage by 1.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2998      +/-   ##
==========================================
+ Coverage   72.36%   73.39%   +1.02%     
==========================================
  Files          99       99              
  Lines        8820     8825       +5     
  Branches     1386     1391       +5     
==========================================
+ Hits         6383     6477      +94     
+ Misses       2005     1929      -76     
+ Partials      432      419      -13
Impacted Files Coverage Δ
torchvision/models/googlenet.py 75.13% <0.00%> (-3.87%) ⬇️
torchvision/transforms/transforms.py 80.51% <0.00%> (-2.07%) ⬇️
torchvision/models/inception.py 85.03% <0.00%> (-2.05%) ⬇️
torchvision/datasets/video_utils.py 77.57% <0.00%> (-0.41%) ⬇️
torchvision/io/image.py 82.25% <0.00%> (-0.36%) ⬇️
torchvision/transforms/functional.py 80.44% <0.00%> (-0.22%) ⬇️
torchvision/datasets/utils.py 64.63% <0.00%> (ø)
torchvision/models/mnasnet.py 75.75% <0.00%> (ø)
torchvision/ops/deform_conv.py 72.30% <0.00%> (ø)
torchvision/models/detection/_utils.py 85.43% <0.00%> (+1.98%) ⬆️
... and 9 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 b8b08ac...f38b4a7. Read the comment docs.

fmassa
fmassa reviewed Nov 13, 2020
View reviewed changes
Copy link
Copy Markdown
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 for the PR Vasilis!

It's great that we are looking into improving the C++ layout / part of torchvision, it's a much-needed feature.

I'll let some C++ experts to comment in here, as my experience with C++ best practices is very limited.

cc @ezyang could you have a look and give suggestions on how we could potentially improve the current setup in torchvision? You've worked a bit on torchvision during the dispatcher refactoring PR, so it might still be fresh in your mind :-)

@ezyang ezyang mentioned this pull request Nov 16, 2020
Merged
@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented Nov 16, 2020

could you have a look and give suggestions on how we could potentially improve the current setup in torchvision?

Most of the renaming stuff doesn't really matter. I did notice the weird camel casing when I originally did some dispatcher refactoring but it's totally an internal implementation detail.

One thing that does matter, and is worth doing if you want to bundle a bunch of code motion up all at once, is getting implementations out of headers and into cpp files. Better header hygiene means faster recompile time which is definitely worth it if you can suffer through having to declare everything twice.

@datumbox datumbox mentioned this pull request Nov 26, 2020
Closed
5 tasks
@datumbox
Use header files:
9026df0 
- Create header files for kernel implementation and remove definitions from vision_*.h files.
- Eliminate unnecessary headers and ensure all cpp include their headers.
@datumbox
Change the naming convention for kernel implementations.
ec11906
@datumbox
Remove the _param postfix from the variables and standardizing names.
f3f8469
@datumbox
Exposing public forward/backward methods to the C++ API and moving me…
da80ce1 
…thods around to minimize git blame changes.
@datumbox datumbox closed this Dec 2, 2020
@datumbox datumbox deleted the refactoring/namespaces_ops branch December 2, 2020 17:44
@datumbox datumbox mentioned this pull request Dec 10, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang left review comments

@fmassa fmassa fmassa left review comments

+1 more reviewer

@bmanga bmanga bmanga left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@datumbox @ezyang @fmassa @bmanga @facebook-github-bot