Implementation for Position-sensitive ROI Pool/Align [updated] #1410
Conversation
|
The failed checks all report that package "six" is missing. |
You would need to create a new PR and do the rebase, which would be a bit more work compared to what you currently have. If @sampepose is ok with this, we could move on with this PR as is.
This is an issue in PyTorch, which will be fixed in pytorch/pytorch#27282 and tomorrow (once the PR has been merged and the new nightly pushed) this should be fixed The general structure looks great to me, thanks a lot! |
|
No worries, you can keep my work in this PR. Just rename the title to reflect that it's not only CPU ops. |
LukasBommes
changed the title
|
Great! I changed the title accordingly. |
fmassa
left a comment
fmassa
left a comment
There was a problem hiding this comment.
The implementation looks great to me, thanks a lot!
Lint is failing for both C++ and Python, can you look into fixing it?
cc @varunagrawal if you'd like to have a look at well.
| dtype=self.dtype) | ||
| assert torch.allclose(gt_y, y), 'PSRoIAlign layer incorrect on CPU' | ||
|
|
||
| def test_ps_roi_align_cpu(self): |
There was a problem hiding this comment.
For a follow-up PR: we should simplify the testing for the roi_* layers, they can be refactored so that we have many less lines of code.
|
@fmassa: I'll take a look at fixing the linter complaints when I am more free. This week I am a bit too busy. |
|
@LukasBommes no worries, I'll also have a closer look next week |
Codecov Report
@@ Coverage Diff @@
## master #1410 +/- ##
=========================================
Coverage ? 64.08%
=========================================
Files ? 82
Lines ? 6362
Branches ? 970
=========================================
Hits ? 4077
Misses ? 2002
Partials ? 283
Continue to review full report at Codecov.
|
fmassa
left a comment
fmassa
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot!
I have to fix CUDA and Windows build first before I can merge the PR, but this PR look like it's ready to be merged.
|
@LukasBommes can you rebase your PR on top of current master? I've issued a fix for the CUDA test, I'd like to see if tests pass on CUDA |
|
@fmassa: Was it correct to just pull from upstream master and merge into my branch? Or is something else needed to rebase? |
|
Your rebase was fine, can you list the commands you did just for completeness? |
|
Thanks a lot! |
|
Welcome! Thanks for merging! |
|
I also agree on the fact that the tests should be refactored in the next step. I simply duplicated the code to prevent introducing errors in the tests. |
4 participants
This implements CPU fallback for PSROIPool and PSROIAlign layers based on their CUDA versions implemented in PR #1259.
See also the related discussion.
@fmassa: This contains the changes you requested in PR #1404.
BTW: Sorry for creating a new PR. Since I checked out a new repo I didn't know how to push to the old repo. Can we reintroduce the commits done by @sampepose? Since I kind of "stole" his work (the CUDA implementation of the two layers) now.