Skip to content

Conversation

sampepose
Copy link

This adds PS ROI pool and align. I also fixed a couple bugs with existing ROI pool tests.

@sampepose sampepose force-pushed the master branch 3 times, most recently from ce3e62d to 9efd51c Compare August 23, 2019 04:59
@codecov-io
Copy link

codecov-io commented Aug 23, 2019

Codecov Report

Merging #1259 into master will decrease coverage by 0.74%.
The diff coverage is 39.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1259      +/-   ##
==========================================
- Coverage   65.89%   65.14%   -0.75%     
==========================================
  Files          75       76       +1     
  Lines        5785     5876      +91     
  Branches      885      886       +1     
==========================================
+ Hits         3812     3828      +16     
- Misses       1708     1783      +75     
  Partials      265      265
Impacted Files Coverage Δ
torchvision/ops/poolers.py 94.02% <100%> (ø) ⬆️
torchvision/ops/__init__.py 100% <100%> (ø) ⬆️
torchvision/ops/ps_roi_align.py 34.04% <34.04%> (ø)
torchvision/ops/ps_roi_pool.py 37.2% <37.2%> (ø)
torchvision/ops/roi_pool.py 67.44% <0%> (-6.64%) ⬇️
torchvision/ops/roi_align.py 65.95% <0%> (-5.48%) ⬇️
torchvision/extension.py 38.09% <0%> (-2.82%) ⬇️
torchvision/utils.py 65.38% <0%> (-0.66%) ⬇️
torchvision/transforms/transforms.py 80.94% <0%> (-0.63%) ⬇️
torchvision/models/shufflenetv2.py 85.54% <0%> (-0.18%) ⬇️
... and 6 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 a129b6b...aa1f5e6. Read the comment docs.

@fmassa fmassa mentioned this pull request Aug 30, 2019
3 tasks
@LukasBommes
Copy link
Contributor

I'm already using the layer and it works fine!

@fmassa
Copy link
Member

fmassa commented Sep 6, 2019

I will get this reviewed and merged into torchvision soon.
But before we need to have CI for windows and OSX working, so that we can make sure that everything is working fine on the OS that we support, see #1298

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 for the PR!

torchvision requires the ops to be implemented on both CPU and GPU (with a potential fallback to CPU in some particular cases, but CPU implementation is a must).

So this PR can't be merged in it's current state unfortunately.

AT_ERROR("Not compiled with GPU support");
#endif
}
AT_ERROR("Not implemented on the CPU");
Copy link
Member

Choose a reason for hiding this comment

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

torchvision follows similar requirements as PyTorch with respect to the implementation of the ops: it requires the ops to be implemented for both CPU and GPU.

@LukasBommes
Copy link
Contributor

@sampepose Are you working on this? Or should I give it a try?

@LukasBommes
Copy link
Contributor

I'm currently working on the CPU implementation as I rely on this layer for another project.

@fmassa
Copy link
Member

fmassa commented Oct 25, 2019

This has been subsumed by #1410. Thanks for the original version @sampepose!

@fmassa fmassa closed this Oct 25, 2019
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