Skip to content

Enhance diagonal (fixes #6479) #6718

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
Merged
merged 4 commits into from
Apr 26, 2018
Merged

Enhance diagonal (fixes #6479) #6718

merged 4 commits into from
Apr 26, 2018

Conversation

t-vi
Copy link
Collaborator

@t-vi t-vi commented Apr 18, 2018

This patch

  • adds Tensor.diagonal to complement torch.diagonal
  • implements diagonal natively in ATen
  • makes diagonal a view (similar to numpy semantics, numpy currently has a read-only view to ease transition from diagonal returning a copy)
  • implements taking arbitrary diagonals with numpy semantics
  • implements diagonal backward instead of referring
    to the (more limited) diag

There is some discussion in #6479 .

@t-vi
Enhance diagonal
7bf93af 
This patch
- adds Tensor.diagonal to complement torch.diagonal
- implements diagonal natively in ATen
- makes diagonal a view
- implements taking arbitrary diagonals
- implements diagonal backward instead of referring
  to the (more limited) diag
ezyang
ezyang reviewed Apr 18, 2018
View reviewed changes
aten/src/ATen/native/TensorShape.cpp
diag_size = std::min(self.size(dim1)+offset, self.size(dim2));
storage_offset -= offset * self.stride(dim1);
}
AT_ASSERT(diag_size > 0, "invalid diagonal offset %zd", offset); // the diagonal offset was too large in magnitude

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

fmassa
fmassa reviewed Apr 18, 2018
View reviewed changes
test/test_torch.py
@@ -1909,6 +1909,26 @@ def _test_diagonal(self, dtype, device):
def test_diagonal(self):
self._test_diagonal(self, dtype=torch.float32, device='cpu')

@unittest.skipIf(not TEST_NUMPY, 'Numpy not found')
def test_diagonal_multidim(self):
x = torch.randn(10, 11, 12, 13)

This comment was marked as off-topic.

Sign in to view

tools/autograd/templates/Functions.cpp Outdated
Tensor diagonal_backward(const Tensor & grad, IntList input_sizes, int64_t offset, int64_t dim1, int64_t dim2) {
auto grad_input = at::zeros(grad.type(), input_sizes);
auto diag = at::diagonal(grad_input, offset, dim1, dim2);
diag.copy_(grad);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

apaszke
apaszke suggested changes Apr 23, 2018
View reviewed changes
test/test_torch.py Outdated
result = torch.diagonal(x, 0, -2, -1)
expected = xn.diagonal(0, -2, -1)
self.assertEqual(expected.shape, result.shape)
self.assertTrue(np.allclose(expected, result.numpy()))

This comment was marked as off-topic.

Sign in to view

test/test_torch.py Outdated
# test that the backward requires grad
# we do this is because diagonal_backward uses inplace
# operations and gradgradcheck does not catch whether
# they works as expected

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

tools/autograd/templates/Functions.cpp Outdated
// copies the diagonal code in aten/src/ATen/native/TensorShape.cpp
// that would be equivalent to
// auto diag = grad_input.diagonal(offset, dim1, dim2);
// when using diagonal, the output is not differentiable twice

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

apaszke
apaszke reviewed Apr 23, 2018
View reviewed changes
torch/_torch_docs.py Outdated
@@ -1305,6 +1313,17 @@
-0.2239
[torch.FloatTensor of size 2]

>>> x = torch.randn(2,5,4,2)
>>> torch.diagonal(x, -1, 1, 2)

This comment was marked as off-topic.

Sign in to view

@t-vi t-vi force-pushed the diagonal_with_dim branch from 67d2734 to a7d0b1f Compare April 23, 2018 14:05
@ezyang
Copy link
Contributor

ezyang commented Apr 23, 2018

@pytorchbot retest this please

@t-vi
Copy link
Collaborator Author

t-vi commented Apr 23, 2018

@ezyang is that my patch or the macos CI that has the bug? I didn't know that anything I did affected the fourier tests...

@t-vi
Copy link
Collaborator Author

t-vi commented Apr 25, 2018

Hi, is there anything I can do to move this forward?
I'd be looking at adding the diagonal feature in einsum when the improved diagonal is available.

@ezyang
Copy link
Contributor

ezyang commented Apr 26, 2018

@pytorchbot retest this please

I don't see how this PR could have triggered this. Let's try again.

@apaszke
Copy link
Contributor

apaszke commented Apr 26, 2018

@pytorchbot retest this please

ssnl
ssnl reviewed Apr 26, 2018
View reviewed changes
torch/_torch_docs.py
>>> x = torch.randn(2, 5, 4, 2)
>>> torch.diagonal(x, offset=-1, dim1=1, dim2=2)

(0 ,.,.) =

This comment was marked as off-topic.

Sign in to view

@ezyang ezyang merged commit 2b44c42 into pytorch:master Apr 26, 2018
@ssnl ssnl mentioned this pull request May 10, 2018
Closed
Jorghi12 pushed a commit to wsttiger/pytorch that referenced this pull request May 10, 2018
@t-vi @Jorghi12
Enhance diagonal (fixes pytorch#6479) (pytorch#6718)
62403cf 
* Enhance diagonal

This patch
- adds Tensor.diagonal to complement torch.diagonal
- implements diagonal natively in ATen
- makes diagonal a view
- implements taking arbitrary diagonals
- implements diagonal backward instead of referring
  to the (more limited) diag

* add tests, copy diagonal code to backward for double differentiability

* improve tests and doc comment. Thank you, Adam!

* Mark diagonal as view function in gen_autograd.py, use simple backward.
weiyangfb pushed a commit to weiyangfb/pytorch that referenced this pull request Jun 11, 2018
@t-vi @weiyangfb
Enhance diagonal (fixes pytorch#6479) (pytorch#6718)
456c524 
* Enhance diagonal

This patch
- adds Tensor.diagonal to complement torch.diagonal
- implements diagonal natively in ATen
- makes diagonal a view
- implements taking arbitrary diagonals
- implements diagonal backward instead of referring
  to the (more limited) diag

* add tests, copy diagonal code to backward for double differentiability

* improve tests and doc comment. Thank you, Adam!

* Mark diagonal as view function in gen_autograd.py, use simple backward.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssnl ssnl ssnl left review comments

@fmassa fmassa fmassa left review comments

@ezyang ezyang ezyang approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

+1 more reviewer

@apaszke apaszke apaszke approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
open source
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@t-vi @ezyang @apaszke @ssnl @fmassa