Fix contiguous AD and Autogradzero inconsistency #18633
Conversation
facebook-github-bot
added
the
oncall: jit
wanchaol
force-pushed
the
autogradzero
branch
2 times, most recently
from
bb43f7c to
657f44b
Compare
wanchaol
changed the title
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@wanchaol has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
The test change for AD support has been merged, would you mind rebasing and see if all tests passed? Thanks a lot! |
test/test_jit.py
Outdated
| def foo(x): | ||
| return 3 + x.contiguous() | ||
|
|
||
| x = torch.rand(1, dtype=torch.float, requires_grad=True) |
There was a problem hiding this comment.
x is contiguous here, we want to test against both when x is /isn't contiguous I think?
ailzhang
left a comment
ailzhang
left a comment
There was a problem hiding this comment.
Thanks!! Please land if CI is green.
torch/csrc/jit/symbolic_script.cpp
Outdated
| if self.is_contiguous(): | ||
| return grad_output | ||
| else: | ||
| return grad_output.clone() |
There was a problem hiding this comment.
You should just return grad_output. If an op downstream needs it to be contiguous, it will do it on its own.
There was a problem hiding this comment.
I guess if we simply return grad_output, it should be fine to make the test simpler.
test/test_jit.py
Outdated
|
|
||
| grad = torch.randn(5, 5, dtype=torch.float) | ||
| out.backward(grad) | ||
| self.assertEqual(x.grad, grad.transpose(1, 0)) |
There was a problem hiding this comment.
Sorry, but what are we testing here? This should be covered as part of autograd tests.
There was a problem hiding this comment.
We are testing the AD formula, the autograd test does not cover the Ad formula I believe.
There was a problem hiding this comment.
In that case, please add an autograd test! It's a differentiable op just like any other.
There was a problem hiding this comment.
yep just added them to autograd tests :)
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@wanchaol has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
wanchaol
dismissed
apaszke’s stale review
fixed the AD formula to only return grad_output, and make test covered in autograd
4 participants
Fixes #17962