Add test for large batches in DeformConv2d #2040
Conversation
fmassa
force-pushed
the
test-large-batch-deform-conv2d
branch
from
793d7bd to
6e6c2da
Compare
Codecov Report
@@ Coverage Diff @@
## master #2040 +/- ##
======================================
Coverage 0.48% 0.48%
======================================
Files 92 92
Lines 7411 7411
Branches 1128 1128
======================================
Hits 36 36
Misses 7362 7362
Partials 13 13Continue to review full report at Codecov.
|
fmassa
force-pushed
the
test-large-batch-deform-conv2d
branch
from
b5aa349 to
252e063
Compare
| @@ -454,7 +454,7 @@ def expected_fn(self, x, weight, offset, bias, stride=1, padding=0, dilation=1): | |||
| return out | |||
|
|
|||
| def get_fn_args(self, device, contiguous): | |||
| batch_sz = 1 | |||
| batch_sz = 33 | |||
There was a problem hiding this comment.
Why not test both batch_sz 1, 33 and some larger value (in case that magic number changes)?
There was a problem hiding this comment.
I would like to change this test to be faster actually -- in it's current state it is the slowest test in torchvision (takes 30s to run). Ideally, I would like to only do gradcheck on a smaller tensor, but still check for correctness in the large-tensor case. My plan was to open an issue to improve this in the future
| {n_in_channels * weight_w * weight_h, n_parallel_imgs * out_h * out_w}, | ||
| input.options()); | ||
|
|
||
| // Separate into blocks | ||
| grad_input = grad_input.view( | ||
| grad_input = grad_input.reshape( |
There was a problem hiding this comment.
Why is this changing from view to reshape?
There was a problem hiding this comment.
There are many changes like that which are mostly artifacts of when I was debugging ongoing issues, and although most of them are not needed per se, I think it's a better practice now to use reshape instead of view, as it works with non-contiguous tensors.
Basically, using reshape here will probably not change anything to the current code-path, but I think it's safer
There was a problem hiding this comment.
It's safer, but it can also incur unexpected memory operations
There was a problem hiding this comment.
totally agree, but if we want to support non-contiguous tensors in this function, we would need to call .contiguous() before anyway, so this becomes a no-op
|
Thanks for the review Christian! |
* Add test for large batches in DeformConv2d * Clean-up and (try) fix DeformConv2d * Simplifications and bugfixes * Try fix CUDA now
Follow-up for #2027