Allow align_to to take in partially named tensors
#27308
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, `tensor.align_to(*names)` has the restriction that the
`tensor` must be fully named. This doesn't need to be the case, when
using Ellipsis, we "expand the ellipsis to all unmentioned dimensions,
in the order which they appear in the original tensor".
For example, consider `tensor: Tensor[None, None, C]`.
`tensor.align_to(C, None, None)` is ambiguous because the user might
have wanted to switch the order of the None dimensions and there is no
way to specify that using this API. However, `tensor.align_to('C', ...)`
isn't ambiguous: we can select the two unnamed dimensions in the order
in which they appear.
To actually implement this, we write a brand-new `align_to(names,
ellipsis_idx)` function in c++ that is separate from the regular
`align_to(names)` implementation. Ideally we would support "..." as a
special name in c++ and combine the two implementations; we'll need to
support "..." in c++ in the future but that requires a bit of extra work.
In this PR, Python processees the ellipsis and then calls the correct
overload.
Test Plan:
- run tests
pytorchbot
added
module: internals
zou3519
added a commit
that referenced
this pull request
Oct 3, 2019
Currently, `tensor.align_to(*names)` has the restriction that the
`tensor` must be fully named. This doesn't need to be the case, when
using Ellipsis, we "expand the ellipsis to all unmentioned dimensions,
in the order which they appear in the original tensor".
For example, consider `tensor: Tensor[None, None, C]`.
`tensor.align_to(C, None, None)` is ambiguous because the user might
have wanted to switch the order of the None dimensions and there is no
way to specify that using this API. However, `tensor.align_to('C', ...)`
isn't ambiguous: we can select the two unnamed dimensions in the order
in which they appear.
To actually implement this, we write a brand-new `align_to(names,
ellipsis_idx)` function in c++ that is separate from the regular
`align_to(names)` implementation. Ideally we would support "..." as a
special name in c++ and combine the two implementations; we'll need to
support "..." in c++ in the future but that requires a bit of extra work.
In this PR, Python processees the ellipsis and then calls the correct
overload.
Test Plan:
- run tests
ghstack-source-id: 330c3e9
Pull Request resolved: #27308
nairbv
reviewed
Oct 4, 2019
| variants: method | ||
| supports_named_tensor: True | ||
|
|
||
| - func: align_to(Tensor(a) self, DimnameList order, int ellipsis_idx) -> Tensor(a) |
Collaborator
There was a problem hiding this comment.
this seems to me like an internal helper function that we shouldn't expose to users
Contributor
Author
There was a problem hiding this comment.
We're not exposing this to users. The Python align_to call wraps this function.
Collaborator
There was a problem hiding this comment.
I thought every interface defined in native_functions.yaml becomes visible to users?
Contributor
Author
There was a problem hiding this comment.
In Python, torch.Tensor is a subclass of torch._C.TensorBase. The native functions interface generates align_to as a method of torch._C.TensorBase. I added an overwrite for Tensor.align_to in the Python definition of the class. Since users only interact with torch.Tensor (and not torch._C.TensorBase), they never see the native functions version of align_to.
Contributor
Author
There was a problem hiding this comment.
The above workflow I described is how you can override a method. We can also override torch functions as well (and we do so in cases where it is difficult to implement functionality in C++ for). One of these is broadcast_tensors : users cannot call the native_functions variant of it directly, they always to through the functional.py call.
Contributor
Author
There was a problem hiding this comment.
There's another long tail of things you can do in the C++ or codegen side to write functions in native_functions.yaml but have them not be directly exposed to Python. I can explain those in more detail if you're interested. The conclusion is: no, native_functions.yaml is not the complete source of truth for how things look in the PyTorch frontend.
Collaborator
There was a problem hiding this comment.
thanks, I think I would like to understand that better at some point. I had another case in a PR where I needed to exclude something from here to avoid exposing it.
nairbv
approved these changes
Oct 7, 2019
Currently, `tensor.align_to(*names)` has the restriction that the
`tensor` must be fully named. This doesn't need to be the case, when
using Ellipsis, we "expand the ellipsis to all unmentioned dimensions,
in the order which they appear in the original tensor".
For example, consider `tensor: Tensor[None, None, C]`.
`tensor.align_to(C, None, None)` is ambiguous because the user might
have wanted to switch the order of the None dimensions and there is no
way to specify that using this API. However, `tensor.align_to('C', ...)`
isn't ambiguous: we can select the two unnamed dimensions in the order
in which they appear.
To actually implement this, we write a brand-new `align_to(names,
ellipsis_idx)` function in c++ that is separate from the regular
`align_to(names)` implementation. Ideally we would support "..." as a
special name in c++ and combine the two implementations; we'll need to
support "..." in c++ in the future but that requires a bit of extra work.
In this PR, Python processees the ellipsis and then calls the correct
overload.
Test Plan:
- run tests
Differential Revision: [D17745179](https://our.internmc.facebook.com/intern/diff/D17745179)
zou3519
added a commit
that referenced
this pull request
Oct 9, 2019
Currently, `tensor.align_to(*names)` has the restriction that the
`tensor` must be fully named. This doesn't need to be the case, when
using Ellipsis, we "expand the ellipsis to all unmentioned dimensions,
in the order which they appear in the original tensor".
For example, consider `tensor: Tensor[None, None, C]`.
`tensor.align_to(C, None, None)` is ambiguous because the user might
have wanted to switch the order of the None dimensions and there is no
way to specify that using this API. However, `tensor.align_to('C', ...)`
isn't ambiguous: we can select the two unnamed dimensions in the order
in which they appear.
To actually implement this, we write a brand-new `align_to(names,
ellipsis_idx)` function in c++ that is separate from the regular
`align_to(names)` implementation. Ideally we would support "..." as a
special name in c++ and combine the two implementations; we'll need to
support "..." in c++ in the future but that requires a bit of extra work.
In this PR, Python processees the ellipsis and then calls the correct
overload.
Test Plan:
- run tests
ghstack-source-id: 780a072
Pull Request resolved: #27308
zdevito
pushed a commit
to zdevito/ATen
that referenced
this pull request
Oct 10, 2019
Summary: Pull Request resolved: pytorch/pytorch#27308 Currently, `tensor.align_to(*names)` has the restriction that the `tensor` must be fully named. This doesn't need to be the case, when using Ellipsis, we "expand the ellipsis to all unmentioned dimensions, in the order which they appear in the original tensor". For example, consider `tensor: Tensor[None, None, C]`. `tensor.align_to(C, None, None)` is ambiguous because the user might have wanted to switch the order of the None dimensions and there is no way to specify that using this API. However, `tensor.align_to('C', ...)` isn't ambiguous: we can select the two unnamed dimensions in the order in which they appear. To actually implement this, we write a brand-new `align_to(names, ellipsis_idx)` function in c++ that is separate from the regular `align_to(names)` implementation. Ideally we would support "..." as a special name in c++ and combine the two implementations; we'll need to support "..." in c++ in the future but that requires a bit of extra work. In this PR, Python processees the ellipsis and then calls the correct overload. Test Plan: - run tests Differential Revision: D17745179 Pulled By: zou3519 fbshipit-source-id: 9fed06d224215cfb7efecd8c002604baab3c45e6
Contributor
thiagocrepaldi
pushed a commit
to thiagocrepaldi/pytorch
that referenced
this pull request
Feb 4, 2020
Summary: Pull Request resolved: pytorch#27308 Currently, `tensor.align_to(*names)` has the restriction that the `tensor` must be fully named. This doesn't need to be the case, when using Ellipsis, we "expand the ellipsis to all unmentioned dimensions, in the order which they appear in the original tensor". For example, consider `tensor: Tensor[None, None, C]`. `tensor.align_to(C, None, None)` is ambiguous because the user might have wanted to switch the order of the None dimensions and there is no way to specify that using this API. However, `tensor.align_to('C', ...)` isn't ambiguous: we can select the two unnamed dimensions in the order in which they appear. To actually implement this, we write a brand-new `align_to(names, ellipsis_idx)` function in c++ that is separate from the regular `align_to(names)` implementation. Ideally we would support "..." as a special name in c++ and combine the two implementations; we'll need to support "..." in c++ in the future but that requires a bit of extra work. In this PR, Python processees the ellipsis and then calls the correct overload. Test Plan: - run tests Differential Revision: D17745179 Pulled By: zou3519 fbshipit-source-id: 9fed06d224215cfb7efecd8c002604baab3c45e6
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
align_toto take in partially named tensors #27308 Allowalign_toto take in partially named tensorsCurrently,
tensor.align_to(*names)has the restriction that thetensormust be fully named. This doesn't need to be the case, whenusing Ellipsis, we "expand the ellipsis to all unmentioned dimensions,
in the order which they appear in the original tensor".
For example, consider
tensor: Tensor[None, None, C].tensor.align_to(C, None, None)is ambiguous because the user mighthave wanted to switch the order of the None dimensions and there is no
way to specify that using this API. However,
tensor.align_to('C', ...)isn't ambiguous: we can select the two unnamed dimensions in the order
in which they appear.
To actually implement this, we write a brand-new
align_to(names, ellipsis_idx)function in c++ that is separate from the regularalign_to(names)implementation. Ideally we would support "..." as aspecial name in c++ and combine the two implementations; we'll need to
support "..." in c++ in the future but that requires a bit of extra work.
In this PR, Python processees the ellipsis and then calls the correct
overload.
Test Plan:
Differential Revision: D17745179