Skip to content

added a check for zero stride #28784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
anjali411 wants to merge 3 commits into from
Closed

added a check for zero stride #28784

anjali411 wants to merge 3 commits into from

Conversation

@anjali411
Copy link
Contributor

@anjali411 anjali411 commented Oct 28, 2019

No description provided.

@anjali411 anjali411 requested review from albanD, ezyang and yf225 October 28, 2019 15:27
@anjali411 anjali411 mentioned this pull request Oct 28, 2019
Closed
@anjali411
Copy link
Contributor Author

anjali411 commented Oct 28, 2019 •
edited
Loading

In response to issue: #27598

@ssnl
Copy link
Collaborator

ssnl commented Oct 28, 2019

Can we instead change is_stride_neg to is_stride_nonpos?

@anjali411
Copy link
Contributor Author

anjali411 commented Oct 28, 2019

@ssnl good point! updated.

albanD
albanD approved these changes Oct 28, 2019
View reviewed changes
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

LGTM

yf225
yf225 reviewed Oct 28, 2019
View reviewed changes
aten/src/ATen/native/Convolution.cpp Outdated
TORCH_CHECK(!params.is_padding_neg(), "negative padding is not supported");
TORCH_CHECK(!params.is_output_padding_neg(), "negative output_padding is not supported");
TORCH_CHECK(!params.is_stride_neg(), "negative stride is not supported");
TORCH_CHECK(!params.is_stride_nonpos(), "non positive stride is not supported");
Copy link
Contributor

@yf225 yf225 Oct 28, 2019

Choose a reason for hiding this comment

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

nit: maybe "non-positive" instead of "non positive"?

Copy link
Contributor Author

@anjali411 anjali411 Oct 28, 2019

Choose a reason for hiding this comment

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

done

facebook-github-bot
facebook-github-bot reviewed Oct 28, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

facebook-github-bot
facebook-github-bot reviewed Oct 28, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 29, 2019

@anjali411 merged this pull request in efbaa8a.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Oct 29, 2019
@anjali411 @facebook-github-bot
added a check for zero stride
e8cdc91 
Summary: Pull Request resolved: pytorch/pytorch#28784

Differential Revision: D18178889

Pulled By: anjali411

fbshipit-source-id: 976810bf3f9def3a8f5ca6885b1e049b831f06f3
@gchanan
Copy link
Contributor

gchanan commented Oct 31, 2019

Some minor feedback on this:

  1. It would be a slight improvement to focus on the effect of the change instead of how the change works. i.e. it's more important that this PR fixes a segfault when the convolution stride == 0 than that it does this by checking.

  2. It's a bit concerning that the code in question from the original PR still exists in THNN. That is, if someone manages to call that code via another path it will still crash. We should also change the check in THNN.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@yf225 yf225 yf225 approved these changes

@albanD albanD albanD approved these changes

@ezyang ezyang Awaiting requested review from ezyang

Assignees

No one assigned

Labels

Merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@anjali411 @ssnl @facebook-github-bot @gchanan @yf225 @albanD @mruberry