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

ROIPool + Dispatcher + Autocast + Code Cleanup #2922

Merged
merged 9 commits into from
Oct 30, 2020

Conversation

datumbox
Copy link
Contributor

@datumbox datumbox commented Oct 28, 2020

part of #2797 and #2796

Implements Dispatcher + Autocast for ROIPool.

I'm also doing some clean up on the original code such as:

  • Remove primitive const declaration from method names.
  • Using references when possible.

@datumbox datumbox requested a review from fmassa October 28, 2020 14:00
@datumbox datumbox changed the title ROIPool code cleanup [WIP] ROIPool code cleanup Oct 28, 2020
@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #2922 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2922   +/-   ##
=======================================
  Coverage   73.41%   73.41%           
=======================================
  Files          99       99           
  Lines        8801     8801           
  Branches     1389     1389           
=======================================
  Hits         6461     6461           
  Misses       1915     1915           
  Partials      425      425           

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 1fe1e11...e8fba5e. Read the comment docs.

@datumbox datumbox changed the title [WIP] ROIPool code cleanup ROIPool + Dispatcher + Autocast + Code Cleanup Oct 30, 2020
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.

Code looks great, thanks!

Can you add some tests for autocast as well?
We have an example of it for RoIAlign in

vision/test/test_ops.py

Lines 298 to 303 in 455cd57

@unittest.skipIf(not torch.cuda.is_available(), "CUDA unavailable")
def test_roi_align_autocast(self):
for x_dtype in (torch.float, torch.half):
for rois_dtype in (torch.float, torch.half):
with torch.cuda.amp.autocast():
self._test_forward(torch.device("cuda"), contiguous=False, x_dtype=x_dtype, rois_dtype=rois_dtype)

torchvision/csrc/ROIPool.h Outdated Show resolved Hide resolved
@fmassa
Copy link
Member

fmassa commented Oct 30, 2020

Discussed with @datumbox offline, in order to avoid conflicts with some currently ongoing PRs he will be sending the tests in a separate PR

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 0125a7d into pytorch:master Oct 30, 2020
@datumbox datumbox deleted the refactor/roipool_cleanup branch October 30, 2020 12:02
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* Fixing types.

* Dispatcher + Autocast.

* + Autograd.

* Formating.

* Fixing return casting with autocast.

* Clean up and refactor ROIPool implementation:
- Remove primitive const declaration from method names.
- Using references when possible.

* Restore include headers.

* New line at end of file.
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* Fixing types.

* Dispatcher + Autocast.

* + Autograd.

* Formating.

* Fixing return casting with autocast.

* Clean up and refactor ROIPool implementation:
- Remove primitive const declaration from method names.
- Using references when possible.

* Restore include headers.

* New line at end of file.
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.

2 participants