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

Enhance diagonal (fixes #6479) #6718

ezyang 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

@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
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

This comment was marked as off-topic.

Sign in to view

test/test_torch.py Outdated

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

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

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