Use round-to-negative division when computing output sizes for convolutions involving striding and dilation. #9640

resistor · 2018-07-20T17:52:54Z

ssnl · 2018-07-20T18:01:34Z

Thanks! This is great! Do you want to fix the im2col, col2im, vol2col and col2vol ones as well? e.g., https://github.com/pytorch/pytorch/blob/master/aten/src/THNN/generic/Im2Col.c#L30-L31

fmassa · 2018-07-20T18:08:27Z

I think we also want to apply a similar patch to THCUNN, and also handle it somewhere where CUDNN is dispatched, as the error message is not great there.

In [1]: import torch

In [2]: conv = torch.nn.Conv2d(1, 1, kernel_size=3, dilation=2, stride=2).cuda()

In [3]: tensor = torch.empty(1, 1, 4, 4).cuda()

In [4]: conv(tensor).shape
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-4-c6df65b7b5a7> in <module>()
----> 1 conv(tensor).shape

~/github/pytorch/torch/nn/modules/module.py in __call__(self, *input, **kwargs)
    475             result = self._slow_forward(*input, **kwargs)
    476         else:
--> 477             result = self.forward(*input, **kwargs)
    478         for hook in self._forward_hooks.values():
    479             hook_result = hook(self, input, result)

~/github/pytorch/torch/nn/modules/conv.py in forward(self, input)
    299     def forward(self, input):
    300         return F.conv2d(input, self.weight, self.bias, self.stride,
--> 301                         self.padding, self.dilation, self.groups)
    302
    303

RuntimeError: CuDNN error: CUDNN_STATUS_BAD_PARAM

In [5]: torch.backends.cudnn.enabled = False

In [6]: conv(tensor).shape
Out[6]: torch.Size([1, 1, 1, 1])

resistor · 2018-07-20T18:22:44Z

@ssnl col2vol and vol2col don't have shape checks at all currently, but I can make the change to im2col and col2im.

resistor · 2018-07-20T21:06:03Z

@pytorchbot test this again

soumith · 2018-07-20T21:18:22Z

@resistor pytorchbot retest this please

resistor · 2018-07-20T21:35:36Z

@pytorchbot retest this please

soumith · 2018-07-21T04:47:39Z

@pytorchbot retest this please

resistor · 2018-07-22T03:34:09Z

@pytorchbot retest this please

ssnl · 2018-07-22T16:06:38Z

Awesome. Could you add a test in test_nn for this?

facebook-github-bot

@ssnl has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ssnl · 2018-07-23T04:43:40Z

@pytorchbot retest this please

facebook-github-bot

@ssnl has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

fmassa · 2018-07-23T14:07:53Z

Are making the same changes for THCUNN as well? The same bevahior happen in cuda tensors

ezyang · 2018-07-23T15:23:53Z

maybe-uninitialized Werrors:

04:47:30 In file included from generic/TemporalRowConvolution.c:1:0,
04:47:30                  from /var/lib/jenkins/workspace/aten/src/TH/THGenerateFloatTypes.h:11,
04:47:30                  from /var/lib/jenkins/workspace/aten/src/THNN/init.cpp:180:
04:47:30 /var/lib/jenkins/workspace/aten/src/THNN/generic/TemporalRowConvolution.c: In function 'void THNN_DoubleTemporalRowConvolution_updateGradInput(THNNState*, THTensor*, THTensor*, THTensor*, THTensor*, THTensor*, THTensor*, int, int, int, bool)':
04:47:30 /var/lib/jenkins/workspace/aten/src/THNN/generic/TemporalRowConvolution.c:359:31: error: 'tgradOutput' may be used uninitialized in this function [-Werror=maybe-uninitialized]
04:47:30    THTensor_(free)(tgradOutput);
04:47:30                                ^
04:47:30 /var/lib/jenkins/workspace/aten/src/THNN/generic/TemporalRowConvolution.c:358:26: error: 'tinput' may be used uninitialized in this function [-Werror=maybe-uninitialized]
04:47:30    THTensor_(free)(tinput);

Should be simple enough to work around.

resistor · 2018-07-23T15:25:09Z

@ezyang Those are existing errors.

resistor · 2018-07-24T00:08:55Z

@fmassa I would prefer to address those in a separate change, as I am not currently setup to test CUDA.

fmassa · 2018-07-24T00:11:06Z

@resistor sounds good to me, but I think it is also very important to have consistent behavior between CPU and CUDA.

…utions involving striding and dilation.

resistor · 2018-07-26T06:16:41Z

@fmassa CUDA changes incorporated.

facebook-github-bot

@resistor has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

pjh5 · 2018-07-26T17:07:58Z

Retrying failing test https://ci.pytorch.org/jenkins/job/caffe2-builds/job/py2-cuda9.0-cudnn7-aten-ubuntu16.04-trigger-test/7301/

ssnl

Looks great!

…utions involving striding and dilation. Summary: Pull Request resolved: pytorch/pytorch#9640 Differential Revision: D8948081 Pulled By: resistor fbshipit-source-id: 06f2e3ad1bdb448be6f36577cb9bd27c884df595

…utions involving striding and dilation. Summary: Pull Request resolved: pytorch#9640 Differential Revision: D8948081 Pulled By: resistor fbshipit-source-id: 06f2e3ad1bdb448be6f36577cb9bd27c884df595

Summary: Fixes #21935 by using the integer floor division that was introduced for convolution shapes in #9640. Without this fix, the pooling operators can produce a 1-element output in cases they shouldn't. Disclaimer: I couldn't properly test it locally (it's not picking up the modified version for some reason). I'm marking this WIP until I checked what the CI tools say... Pull Request resolved: #22304 Differential Revision: D16181955 Pulled By: ezyang fbshipit-source-id: a2405372753572548b40616d1206848b527c8121

Summary: Fixes pytorch/pytorch#21935 by using the integer floor division that was introduced for convolution shapes in pytorch/pytorch#9640. Without this fix, the pooling operators can produce a 1-element output in cases they shouldn't. Disclaimer: I couldn't properly test it locally (it's not picking up the modified version for some reason). I'm marking this WIP until I checked what the CI tools say... Pull Request resolved: pytorch/pytorch#22304 Differential Revision: D16181955 Pulled By: ezyang fbshipit-source-id: a2405372753572548b40616d1206848b527c8121

resistor requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners July 20, 2018 17:52

resistor force-pushed the div-rtn branch from 69f4067 to dca2ad9 Compare July 20, 2018 20:17

facebook-github-bot reviewed Jul 22, 2018

View reviewed changes

resistor force-pushed the div-rtn branch from dca2ad9 to 38b94d1 Compare July 23, 2018 04:37

facebook-github-bot reviewed Jul 23, 2018

View reviewed changes

ezyang mentioned this pull request Jul 23, 2018

remove unnecessary -Wno= flags #9608

Closed

resistor force-pushed the div-rtn branch 2 times, most recently from 3156543 to c54f112 Compare July 25, 2018 22:28

Use round-to-negative division when computing output sizes for convol…

1f9df4e

…utions involving striding and dilation.

resistor force-pushed the div-rtn branch from c54f112 to 1f9df4e Compare July 26, 2018 06:15

facebook-github-bot reviewed Jul 26, 2018

View reviewed changes

ssnl approved these changes Jul 26, 2018

View reviewed changes

ezyang approved these changes Jul 27, 2018

View reviewed changes

facebook-github-bot closed this in 6ed41ad Jul 27, 2018

ezyang added the merged label Jun 26, 2019

f0k mentioned this pull request Jun 27, 2019

Use integer floor division for pooling shape computation #22304

Closed

Use round-to-negative division when computing output sizes for convolutions involving striding and dilation. #9640

Use round-to-negative division when computing output sizes for convolutions involving striding and dilation. #9640

Uh oh!

Conversation

resistor commented Jul 20, 2018

Uh oh!

ssnl commented Jul 20, 2018

Uh oh!

fmassa commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

resistor commented Jul 20, 2018

Uh oh!

resistor commented Jul 20, 2018

Uh oh!

soumith commented Jul 20, 2018

Uh oh!

resistor commented Jul 20, 2018

Uh oh!

soumith commented Jul 21, 2018

Uh oh!

resistor commented Jul 22, 2018

Uh oh!

ssnl commented Jul 22, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ssnl commented Jul 23, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

fmassa commented Jul 23, 2018

Uh oh!

ezyang commented Jul 23, 2018

Uh oh!

resistor commented Jul 23, 2018

Uh oh!

resistor commented Jul 24, 2018

Uh oh!

fmassa commented Jul 24, 2018

Uh oh!

resistor commented Jul 26, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

pjh5 commented Jul 26, 2018

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fmassa commented Jul 20, 2018 •

edited

Loading