Add diag_embed to ATen and torch #12447

t-vi · 2018-10-08T07:24:31Z

ssnl

I'm still a bit unsure about the name and the API.

On name: it works on higher rank tensors than 2. So matrix is a bit misleading.
On API: Wouldn't it be nice if users can specify which dimension is used as the diagonal?

torch/_torch_docs.py

+Example::
+
+    >>> x = torch.randn(2, 3)
+    tensor([[[ 1.5410,  0.0000,  0.0000],


torch/_torch_docs.py

+           r"""
+matrix_diag(input, offset=0, dim1=-2, dim2=-1) -> Tensor
+
+Creates a diagonal matrix from :attr:`input` by using the last


aten/src/ATen/native/native_functions.yaml

 - func: matmul_out(Tensor result, Tensor self, Tensor other) -> Tensor

+- func: matrix_diag(Tensor self, int64_t offset=0, int64_t dim1=-2, int64_t dim2=-1) -> Tensor
+  variants: function, method


t-vi · 2018-10-08T07:54:51Z

On the naming: Indeed, I agree that matrix_diag is a not-so-nice name. I would have called that diagonal_tensor if I had to come up with my own name. As such I'll be more than happy to change the name.

On the API: I think that diagonal and this function should match, this is why I took the last dimension. I would expect this to cover the by far most common case.
When I implemented diagonal, I thought it was strange to always put the diagonal at the end, but that is what numpy did. I haven't, however, heard anyone complain, so adding another parameter might be overthinking the applicability of this function. One could always add a "source dimension" keyword argument later (unless that does bad things to the C++ API). Again, I'll gladly add an argument (and would suggest to do the same in torch.diagonal).

apaszke · 2018-10-08T09:24:37Z

Yeah, matrix_diag doesn't seem like a good name if this is supposed to be a general purpose function. Some names from me:

diagfill(...) - kind of like diagflat, except doesn't flatten the input?
as_diagonal(...) - might be confusing, because it looks a bit if it's making the input diagonal

Is there really no term for making a diagonal matrix out of a vector? 😕

t-vi · 2018-10-08T09:39:36Z

Is there really no term for making a diagonal matrix out of a vector?

I'm sure the category theory-inclined will love diagonal_morphism or diagonal_embedding, but we would have to keep people searching for these from looking at Wikipedia.

Actually diagonal_embedding would sound like that to me.

apaszke · 2018-10-08T16:07:12Z

Yeah, "diagonal embedding" seems to be a good term (the morphism could really go both ways, so it remains unclear). It's only a bit verbose. How about diag_embed/embed_diag?

ssnl · 2018-10-08T16:09:59Z

One vote from me for embed_diag, although keeping the same diag_ prefix would be nice.

apaszke · 2018-10-08T16:44:09Z

Yeah, diag_* is definitely good for discoverability, but it's a much worse method name 😕 Can't really say which one is better, but I think diag_embed is better. We can't expect people to go through 400 functions we provide.

fmassa · 2018-10-08T16:48:17Z

About the naming: I suppose numpy doesn't have an equivalent function? FWIW matrix_diag is the name if the function in TF

apaszke · 2018-10-08T17:09:15Z

matrix_diag is really not a good name... The function works with any tensors, and the name doesn't make it clear if it takes the diagonal, or fills the diagonal of a higher-dim tensor.

Also improve doc and example. Thank you all for your input!

t-vi · 2018-10-14T21:28:01Z

So I renamed to diag_embed.

bddppq · 2018-10-15T04:12:46Z

@pytorchbot retest this please

t-vi · 2018-10-15T10:27:05Z

I cannot quite understand the error on rocm:

22:36:15 test_ByteTensor_mode (__main__.TestCuda) ... python: symbol lookup error: /var/lib/jenkins/.local/lib/python2.7/site-packages/torch/lib/libcaffe2_hip.so: undefined symbol: THCudaByteTensor_mode

For the other error, I think I stayed clear of the networking code with diag_embed, but:

Oct 14 21:47:04     store = TCPStore(master_addr, master_port, start_daemon)
Oct 14 21:47:04 RuntimeError: Address already in use

I'd think the PR is OK unless you say it isn't.

ssnl

Code looks clean, as always. However, I do have some comments on the doc.

torch/_torch_docs.py

+The size of the new matrix will be calculated to make the specified diagonal
+of the size of the last input dimension.
+Note that for :attr:`offset` other than :math:`0`, the order of :attr:`dim1`
+and :attr:`dim2` matters, exchaning them is equivalent to changing the


torch/_torch_docs.py

+           r"""
+diag_embed(input, offset=0, dim1=-2, dim2=-1) -> Tensor
+
+Creates a tensor with entries on a diagonal from :attr:`input` by


@ssnl

Thank you, @ssnl, for the suggestions!

t-vi · 2018-10-24T11:54:33Z

I think both CI failures are unrelated to the patch.

t-vi · 2018-10-30T10:54:04Z

@ssnl @apaszke anything I can do to move this forward?

facebook-github-bot

@soumith has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ssnl

I am so sorry. I had comments typed but forgot to hit "submit review" button....

torch/_torch_docs.py

+The size of the new matrix will be calculated to make the specified diagonal
+of the size of the last input dimension.
+Note that for :attr:`offset` other than :math:`0`, the order of :attr:`dim1`
+and :attr:`dim2` matters, exchanging them is equivalent to changing the


torch/_torch_docs.py

+diag_embed(input, offset=0, dim1=-2, dim2=-1) -> Tensor
+
+Creates a tensor whose diagonals of certain 2D planes (specified by
+:attr:`dim1` and :attr:`dim2`) are filled by :attr:`input`. By default, the


ssnl

Thank you so much!

ssnl · 2018-11-04T20:05:19Z

hmm could you rebase? It seems that the base commit is in bad state. Thanks!

facebook-github-bot

@soumith has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@ssnl has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Fixes: #12160 Pull Request resolved: pytorch/pytorch#12447 Differential Revision: D12916234 Pulled By: SsnL fbshipit-source-id: 512a04efb0c2e0a54295b857a61be66c3aae13da

Add matrix_diag to ATen and torch

00c484e

Fixes: pytorch#12160

t-vi requested review from apaszke, colesbury, ezyang, gchanan, soumith, ssnl, zdevito and zou3519 as code owners October 8, 2018 07:24

ssnl reviewed Oct 8, 2018

View reviewed changes

t-vi added 2 commits October 14, 2018 22:43

Merge branch 'master' into matrix_diag

ea16328

Rename to diag_embed

41f501b

Also improve doc and example. Thank you all for your input!

t-vi changed the title ~~Add matrix_diag to ATen and torch~~ Add diag_embed to ATen and torch Oct 14, 2018

ssnl reviewed Oct 15, 2018

View reviewed changes

t-vi added 2 commits October 15, 2018 20:47

improve docstring

b7f6758

Thank you, @ssnl, for the suggestions!

add test in the new place

be5dc2b

soumith approved these changes Nov 3, 2018

View reviewed changes

facebook-github-bot reviewed Nov 3, 2018

View reviewed changes

ssnl reviewed Nov 4, 2018

View reviewed changes

better docs, thanks SSnL!

c777a09

ssnl approved these changes Nov 4, 2018

View reviewed changes

Merge branch 'master' into matrix_diag

a2107a6

facebook-github-bot reviewed Nov 5, 2018

View reviewed changes

facebook-github-bot closed this in f0ed927 Nov 5, 2018

ezyang added open source merged labels Jun 24, 2019

Add diag_embed to ATen and torch #12447

Add diag_embed to ATen and torch #12447

Uh oh!

Conversation

t-vi commented Oct 8, 2018

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

t-vi commented Oct 8, 2018

Uh oh!

apaszke commented Oct 8, 2018

Uh oh!

t-vi commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apaszke commented Oct 8, 2018

Uh oh!

ssnl commented Oct 8, 2018

Uh oh!

apaszke commented Oct 8, 2018

Uh oh!

fmassa commented Oct 8, 2018

Uh oh!

apaszke commented Oct 8, 2018

Uh oh!

t-vi commented Oct 14, 2018

Uh oh!

bddppq commented Oct 15, 2018

Uh oh!

t-vi commented Oct 15, 2018

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

t-vi commented Oct 24, 2018

Uh oh!

t-vi commented Oct 30, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

ssnl commented Nov 4, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

t-vi commented Oct 8, 2018 •

edited

Loading