Skip to content

Fix tensor.permute(dims) backward for negative dims #5945

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

Merged
merged 1 commit into from
Mar 22, 2018

Conversation

zou3519
Copy link
Contributor

@zou3519 zou3519 commented Mar 22, 2018

Fixes #5943

For the following code:

import torch

u = torch.zeros((3, 3), requires_grad=True)
v = u.permute(-1, -2)  # (1, 0) here is fine
v.sum().backward()

during the backward pass, a std::vector is constructed
as an "inverse" of the permutation. To do this, all the dims
are indexed into the vector.

The problem with that is that the negative dims were being indexed
into the std::vector, causing undefined behavior. This PR wraps
those negative dims so they're handled correctly.

cc @colesbury

Test Plan

new unit test

Fixes pytorch#5943

For the following code:
```
import torch

u = torch.zeros((3, 3), requires_grad=True)
v = u.permute(-1, -2)  # (1, 0) here is fine
v.sum().backward()
```

during the backward pass, a std::vector is constructed
as an "inverse" of the permutation. To do this, all the dims
are indexed into the vector.

The problem with that is that the negative dims were being indexed
into the std::vector, causing undefined behavior. This PR wraps
those negative dims so they're handled correctly.
@zou3519
Copy link
Contributor Author

zou3519 commented Mar 22, 2018

@yf225 should I be worried about the CPU perf test? I don't think mnist uses permute but I'm also not sure I'm looking at the right thing.

@yf225
Copy link
Contributor

yf225 commented Mar 22, 2018

@zou3519 The perf test result looks really weird.. I just triggered a retest (https://ci.pytorch.org/jenkins/job/pytorch-builds/job/short-perf-test-cpu/1893/, it might still be in the queue) and we should probably see how that one goes

@zou3519
Copy link
Contributor Author

zou3519 commented Mar 22, 2018

@yf225
Copy link
Contributor

yf225 commented Mar 22, 2018 via email

@soumith soumith merged commit c9c978d into pytorch:master Mar 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants