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

Minor cleanup of roi_align_forward_kernel_impl #3619

Merged
merged 4 commits into from
Mar 31, 2021

Conversation

NicolasHug
Copy link
Member

We can directly pass n_rois instead of using the intermediate n_threads and output_size names

@datumbox
Copy link
Contributor

datumbox commented Mar 30, 2021

I believe that the original author of the method chose to use nthreads to keep the signature the same as roi_align_backward_kernel_impl(). Also note that the same approach is followed at the ps_roi_align_kernel.cpp file.

@NicolasHug
Copy link
Member Author

NicolasHug commented Mar 31, 2021

I think you're right about the original intent, but IMHO the only functions that need to have a common signature are the [ps_]roi_align_forward_kernel, for the dispatcher. The rest can be considered as an implementation detail. Thanks for catching ps_roi_align, I updated that too!

@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #3619 (e061359) into master (591c899) will not change coverage.
The diff coverage is n/a.

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

@@           Coverage Diff           @@
##           master    #3619   +/-   ##
=======================================
  Coverage   79.69%   79.69%           
=======================================
  Files         105      105           
  Lines        9824     9824           
  Branches     1583     1583           
=======================================
  Hits         7829     7829           
  Misses       1517     1517           
  Partials      478      478           
Impacted Files Coverage Δ
torchvision/transforms/functional.py 81.85% <0.00%> (ø)
torchvision/transforms/transforms.py 84.30% <0.00%> (ø)

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 591c899...02ea31d. 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.

Fine with me, as we can consider this as an implementation detail.

@fmassa fmassa merged commit 02a1918 into pytorch:master Mar 31, 2021
facebook-github-bot pushed a commit that referenced this pull request Apr 13, 2021
Summary:
* minor clean up

* do same for ps_roialign

Reviewed By: NicolasHug

Differential Revision: D27706957

fbshipit-source-id: 3320466f6a8b12445f4c901460d3b6f39e6760ea

Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
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.

4 participants