Report convolution size mismatch #17436

bhushan23 · 2019-02-23T22:23:33Z

Kernel size is larger than input
Expected output size to be less than zero

Test case added:

invalid_conv1d
Relevant test cases for conv2d and conv3d exists

bhushan23 · 2019-02-24T00:12:10Z

@fmassa I couldn't verify the fix locally for cuda due to logistic reasons.
Is it possible for you to pull this and validate?

bhushan23 · 2019-02-25T20:45:09Z

@fmassa could you please review?

mrshenli · 2019-02-26T19:10:36Z

aten/src/ATen/native/Convolution.cpp

Correct me if I am wrong, I don't think we support negative stride on convolutional layers. If so, can you add an AT_CHECK for that? And if so, do you still need the output_shape check?

>>> c = nn.Conv1d(3, 2, kernel_size=7, stride=1) >>> c(torch.zeros(2, 3, 10)) tensor([[[-0.1358, -0.1358, -0.1358, -0.1358], [-0.0659, -0.0659, -0.0659, -0.0659]], [[-0.1358, -0.1358, -0.1358, -0.1358], [-0.0659, -0.0659, -0.0659, -0.0659]]], grad_fn=<SqueezeBackward1>) >>> c = nn.Conv1d(3, 2, kernel_size=7, stride=-1) >>> c(torch.zeros(2, 3, 10)) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/shenli/project/pytorch/torch/nn/modules/module.py", line 491, in __call__ result = self.forward(*input, **kwargs) File "/home/shenli/project/pytorch/torch/nn/modules/conv.py", line 188, in forward self.padding, self.dilation, self.groups) RuntimeError: Trying to create tensor with negative dimension -2: [2, 2, 1, -2] >>> c = nn.Conv1d(3, 2, kernel_size=7, stride=-3) >>> c(torch.zeros(2, 3, 10)) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/home/shenli/project/pytorch/torch/nn/modules/module.py", line 491, in __call__ result = self.forward(*input, **kwargs) File "/home/shenli/project/pytorch/torch/nn/modules/conv.py", line 188, in forward self.padding, self.dilation, self.groups)

we don't support negative strides. Added check for it.
Handling dilation for computing kernel size which leads to no need of output shape hence removing.

mrshenli · 2019-02-26T19:13:23Z

aten/src/ATen/native/Convolution.cpp

Can this two lines be moved into the check_input_shape_forward method?

mrshenli · 2019-02-26T19:18:43Z

test/test_nn.py

To close #17247, could you please add all the reported failed use cases to the test?

We already have other test cases e.g. invalid_conv2d and 3d which does check for current error

fmassa

Hi,

Thanks for the PR!

I have a few questions, let me know what you think.

fmassa · 2019-02-26T19:13:32Z

test/test_nn.py

Can you remove this print?

fmassa · 2019-02-26T19:21:17Z

aten/src/ATen/native/Convolution.cpp

I believe dilation should also be taken into account here

@fmassa do we claim to support negative stride for convolutional layers?

I agree with @mrshenli 's previous comment of not allowing negative stride which removes the need of output_shape only if we consider dilation for updating new kernel size and use it for input shape mismatch

Sounds good to me

fmassa · 2019-02-26T19:21:38Z

aten/src/ATen/native/Convolution.cpp

output_padding is only used in transposed convolutions

dropping comment

fmassa · 2019-02-26T19:23:35Z

aten/src/ATen/native/Convolution.cpp

do we need to do similar checks for transposed as well?

No. transposed convolution are not dependent on kernel size and input shape. output will determine on kernel size but not restricted on kernel size.

fmassa · 2019-03-06T20:02:48Z

@pytorchbot rebase this please

pytorchbot · 2019-03-06T20:02:52Z

Sorry, I can't merge this because there are conflicts. To merge this yourself, run the commands below:

git fetch origin master
git fetch git@github.com:bhushan23/pytorch.git conv
git checkout FETCH_HEAD
git merge origin/master
git push git@github.com:bhushan23/pytorch.git HEAD:conv

(To learn more about this bot, see Bot commands.)

fmassa · 2019-03-06T20:03:16Z

@bhushan23 can you fix the conflicts?

bhushan23 · 2019-03-06T20:18:06Z

Thanks @fmassa
conflit resolved

facebook-github-bot

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

mrshenli

LGTM, only 2 nitpicks

mrshenli · 2019-03-07T02:07:02Z

aten/src/ATen/native/Convolution.cpp

it seems we no longer need this header any more. Can we remove it?

mrshenli · 2019-03-07T02:08:17Z

aten/src/ATen/native/Convolution.cpp

unused var, please remove.

- Considering dilation and computing new kernel size for input checking - Refactored check_input_shape_forward as check_shape_forward as adding padding, stride check - Ensuring stride to be non-zero Test case added: - invalid_conv1d - relevant test cases for conv2d and conv3d exists - added test cases for negative stride

bhushan23 · 2019-03-07T04:54:21Z

thanks @mrshenli
I totally missed unused var and include. Removed now.

mrshenli · 2019-03-07T09:29:55Z

@pytorchbot retest this please

facebook-github-bot

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

facebook-github-bot

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

Summary: 1. Kernel size is larger than input 2. Expected output size to be less than zero Test case added: - invalid_conv1d - Relevant test cases for conv2d and conv3d exists Fixes #17247 Pull Request resolved: pytorch/pytorch#17436 Reviewed By: mrshenli Differential Revision: D14354272 Pulled By: fmassa fbshipit-source-id: 94b98621aa03b1f60d151ef9399ed3da55d41b42

iotamudelta · 2019-06-27T18:49:32Z

@bhushan23 @ezyang this PR removed support for using MIOpen on ROCm for depthwise convolutions (line 357). Hence, performance for models using them regressed on ROCm. How can we go about re-instantiating support and ensure support remains in?

bhushan23 · 2019-07-10T05:56:41Z

@bhushan23 @ezyang this PR removed support for using MIOpen on ROCm for depthwise convolutions (line 357). Hence, performance for models using them regressed on ROCm. How can we go about re-instantiating support and ensure support remains in?

@iotamudelta we need ton add conditional miopen depthwise convolution back.
back in time, we thought of using one path for conv.

bhushan23 · 2019-07-10T05:59:47Z

@iotamudelta how much performance drop has been observed?
It is good time to access and understand perf gain we are getting with the miopen implementation

bhushan23 mentioned this pull request Feb 24, 2019

Convolution layer cann't report size error when padded input size is less than kernel size #17247

Closed

mrshenli self-requested a review February 26, 2019 17:28

mrshenli reviewed Feb 26, 2019

View reviewed changes

fmassa reviewed Feb 26, 2019

View reviewed changes

bhushan23 closed this Mar 1, 2019

bhushan23 reopened this Mar 1, 2019

fmassa approved these changes Mar 6, 2019

View reviewed changes

facebook-github-bot reviewed Mar 6, 2019

View reviewed changes

mrshenli approved these changes Mar 7, 2019

View reviewed changes

facebook-github-bot reviewed Mar 8, 2019

View reviewed changes

facebook-github-bot reviewed Mar 12, 2019

View reviewed changes

facebook-github-bot closed this in 16e50c7 Mar 14, 2019

pytorchbot added the merged label Mar 14, 2019

ezyang added the open source label Jun 24, 2019

Report convolution size mismatch #17436

Report convolution size mismatch #17436

Uh oh!

Conversation

bhushan23 commented Feb 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhushan23 commented Feb 24, 2019

Uh oh!

bhushan23 commented Feb 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhushan23 Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmassa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmassa commented Mar 6, 2019

Uh oh!

pytorchbot commented Mar 6, 2019

Uh oh!

fmassa commented Mar 6, 2019

Uh oh!

bhushan23 commented Mar 6, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

mrshenli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhushan23 commented Mar 7, 2019

Uh oh!

mrshenli commented Mar 7, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

iotamudelta commented Jun 27, 2019

Uh oh!

bhushan23 commented Jul 10, 2019

Uh oh!

bhushan23 commented Feb 23, 2019 •

edited

Loading

bhushan23 Mar 1, 2019 •

edited

Loading