-
Notifications
You must be signed in to change notification settings - Fork 24.9k
Fix max_pool2d for returning wrong shape with return_indices=True on cuda #38992
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.
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
Conversation
💊 CI failures summary and remediationsAs of commit e32253d (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 7 times. |
test/test_nn.py
Outdated
def test_max_pool2d_indices(self, device): | ||
def helper(n, c, h, w, ks): | ||
if h is None: | ||
x = torch.randn(n, c, w, device='cuda', dtype=torch.float, requires_grad=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test (while correct) is confusing - 3d input is chw, not ncw (that it, it lacks batch dimension, not one of the spatial dimensions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, seems like I confused that with the unsqueeze(2) in max_pool1d. Will fix. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good, but please fix variables in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ngimel has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Fix #38986
The current code only resizes pooling output but forget to resize indices as well.