Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug in max_pool2d with ceil_mode under certain conditions #45357

Closed
knottb opened this issue Sep 25, 2020 · 4 comments
Closed

Bug in max_pool2d with ceil_mode under certain conditions #45357

knottb opened this issue Sep 25, 2020 · 4 comments
Assignees
@heitorschueroff
Labels
high priority module: nn Related to torch.nn module: pooling triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Milestone
1.8.0

Comments

@knottb
Copy link

knottb commented Sep 25, 2020 •
edited by pytorch-probot bot

馃悰 Bug

In torch.nn.functional.max_pool2d with ceil_mode=True, there are cases where maximum can be taken for a fully out-of-bounds kernel. See repro.

To Reproduce

>>> x = torch.randn(1, 1, 6, 7)
>>> y = torch.nn.functional.max_pool2d(x, 1, stride=(2, 2), padding=0, ceil_mode=True)
>>> y.size()
torch.Size([1, 1, 4, 4])
>>> y
tensor([[[[ 1.5680,  1.0815, -0.5165, -1.0466],
          [ 1.2615,  1.1358, -0.0340, -0.4750],
          [ 0.7457, -1.0273,  1.0388,  0.7754],
          [   -inf,    -inf,    -inf,    -inf]]]])

Expected behavior

I would expect for these parameters (kernel_size=1, stride=(2, 2), ceil_mode=True), we would get an output of size (3, 4) since the 4th kernel on dimension 2 would start at index 6 which is out-of-bounds.

cc @ezyang @gchanan @zou3519 @albanD @mruberry
@AnshuTrivedi
Copy link

@knottb I think it's not bug .For reference see here.
In the case of ceil mode, additional columns and rows are added at the right as well as at the down. (Not top and not left). It does not need to be one extra column. It depends on the stride value as well.

@gchanan
Copy link
Contributor

gchanan commented Sep 28, 2020

A quick reading of the docs and napkin math suggests it should be (3,4) so either the code or the docs are wrong.

@gchanan gchanan added module: nn Related to torch.nn triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module high priority labels Sep 28, 2020
@heitorschueroff heitorschueroff self-assigned this Oct 5, 2020
@ezyang
Copy link
Contributor

ezyang commented Oct 5, 2020

@ngimel thinks the code is wrong, because a completely out of bounds window is not right

@gchanan
Copy link
Contributor

gchanan commented Oct 14, 2020

confirmed this isn't a regression, so putting on the 1.8.0 milestone.

@gchanan gchanan added this to the 1.8.0 milestone Oct 14, 2020
@heitorschueroff heitorschueroff mentioned this issue Oct 19, 2020
Closed
1 task
@heitorschueroff heitorschueroff linked a pull request Oct 19, 2020 that will close this issue
Fix max_pool2d with ceil_mode bug #46558
Closed
1 task
heitorschueroff added a commit that referenced this issue Oct 20, 2020
@heitorschueroff
Update on "Fix max_pool2d with ceil_mode bug"
a61eb98 
This PR fixes a bug with how pooling output shape was computed. Previously, it did not consider the right padding as "part of the image" but the docs do. Now, in ceil mode it allows a kernel window to go off bounds as long as it starts within the image including both paddings. The boolean condition was also wrong, it should be ceil_mode.

fixes #45357

[ghstack-poisoned]
heitorschueroff added a commit that referenced this issue Oct 21, 2020
@heitorschueroff
Update on "Fix max_pool2d with ceil_mode bug"
c658fd0 
This PR fixes a bug with how pooling output shape was computed. Previously, it did not consider the right padding as "part of the image" but the docs do. Now, in ceil mode it allows a kernel window to go off bounds as long as it starts within the image including both paddings. The boolean condition was also wrong, it should be ceil_mode.

fixes #45357

TODO

- [ ] Ensure existing tests are checking for the correct output size

[ghstack-poisoned]
heitorschueroff added a commit that referenced this issue Oct 23, 2020
@heitorschueroff
Update on "Fix max_pool2d with ceil_mode bug"
4f50e6e 
This PR fixes a bug with how pooling output shape was computed. 

## BC Breaking Notes
Previously, the pooling code allowed a kernel window to be entirely outside the input and it did not consider right padding as part of the input in the computations. Now, in ceil mode it allows a kernel window to go off bounds as long as it starts within the input including both paddings. If ceil_mode=False, then all kernel windows have to start inside the input, again including both paddings.

fixes #45357

TODO

- [ ] Ensure existing tests are checking for the correct output size

[ghstack-poisoned]
heitorschueroff added a commit that referenced this issue Oct 27, 2020
@heitorschueroff
Update on "Fix max_pool2d with ceil_mode bug"
c917ad7 
This PR fixes a bug with how pooling output shape was computed. 

## BC Breaking Notes
Previously, the pooling code allowed a kernel window to be entirely outside the input and it did not consider right padding as part of the input in the computations. Now, in ceil mode it allows a kernel window to go off bounds as long as it starts within the input including both paddings. If ceil_mode=False, then all kernel windows have to start inside the input, again including both paddings.

fixes #45357

TODO

- [x] Ensure existing tests are checking for the correct output size

[ghstack-poisoned]
heitorschueroff added a commit that referenced this issue Oct 27, 2020
@heitorschueroff
Update on "Fix max_pool2d with ceil_mode bug"
589e872 
This PR fixes a bug with how pooling output shape was computed. 

## BC Breaking Notes
Previously, the pooling code allowed a kernel window to be entirely outside the input and it did not consider right padding as part of the input in the computations. Now, in ceil mode it allows a kernel window to go off bounds as long as it starts within the input including both paddings. If ceil_mode=False, then all kernel windows have to start inside the input, again including both paddings.

fixes #45357

TODO

- [x] Ensure existing tests are checking for the correct output size

[ghstack-poisoned]
heitorschueroff added a commit that referenced this issue Oct 27, 2020
@heitorschueroff
Update on "Fix max_pool2d with ceil_mode bug"
4009646 
This PR fixes a bug with how pooling output shape was computed.

## BC Breaking Notes
Previously, a bug in the pooling code allowed a sliding window to be entirely off bounds. Now, sliding windows must start inside the input or left padding (not right padding, see #46929) and may only go off-bounds if ceil_mode=True.

fixes #45357

TODO

- [x] Ensure existing tests are checking for the correct output size

[ghstack-poisoned]
heitorschueroff added a commit that referenced this issue Oct 29, 2020
@heitorschueroff
Update on "Fix max_pool2d with ceil_mode bug"
a4f0899 
This PR fixes a bug with how pooling output shape was computed.

## BC Breaking Notes
Previously, a bug in the pooling code allowed a sliding window to be entirely off bounds. Now, sliding windows must start inside the input or left padding (not right padding, see #46929) and may only go off-bounds if ceil_mode=True.

fixes #45357

TODO

- [x] Ensure existing tests are checking for the correct output size

[ghstack-poisoned]
@xwang233 xwang233 mentioned this issue Apr 28, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@heitorschueroff heitorschueroff

Labels
high priority module: nn Related to torch.nn module: pooling triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
1.8.0
Development

Successfully merging a pull request may close this issue.

Fix max_pool2d with ceil_mode bug pytorch/pytorch
5 participants
@ezyang @gchanan @heitorschueroff @knottb @AnshuTrivedi