named tensor support for torch.equal #29322
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
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests [ghstack-poisoned]
This was referenced Nov 6, 2019
zou3519
added a commit
that referenced
this pull request
Nov 6, 2019
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests ghstack-source-id: 9e05747 Pull Request resolved: #29322
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests [ghstack-poisoned]
zou3519
added a commit
that referenced
this pull request
Nov 6, 2019
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests ghstack-source-id: 5c0d544 Pull Request resolved: #29322
nairbv
reviewed
Nov 7, 2019
| return equal; | ||
| } | ||
|
|
||
| int THTensor_(equal)(THTensor *ta, THTensor* tb) { |
Collaborator
There was a problem hiding this comment.
it looks like this is only called from functions like bool _th_equal in LegacyTHFunctionsCPU.cpp. Would it be easy to return a bool here (and in the cuda version) instead of returning an int and casting farther up the stack?
Contributor
Author
There was a problem hiding this comment.
The original function returned an int so I am following that convention. I don't know the answer to whether or not a bool return here changes the behavior.
nairbv
reviewed
Nov 7, 2019
| } | ||
|
|
||
| bool are_names_equal(TensorImpl* self, TensorImpl* other) { | ||
| if (!impl::has_names(self) && !impl::has_names(other)) { |
Collaborator
There was a problem hiding this comment.
nit: I don't know if this is better since it might be less clear to read, but I'd be tempted to put:
if (!impl::has_names(self)) {
return !impl::has_names(other);
}
short-circuits the case where they're not equal and only the second arg has names.
Contributor
Author
There was a problem hiding this comment.
That's not correct. If other doesn't have names and self does have names, their names can still be equal if all of self's names are None.
nairbv
approved these changes
Nov 7, 2019
Collaborator
nairbv
left a comment
nairbv
left a comment
There was a problem hiding this comment.
minor suggestions but looks good
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests [ghstack-poisoned]
zou3519
added a commit
that referenced
this pull request
Nov 11, 2019
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests ghstack-source-id: 71151c3 Pull Request resolved: #29322
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests [ghstack-poisoned]
zou3519
added a commit
that referenced
this pull request
Nov 11, 2019
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests ghstack-source-id: f61b2a6 Pull Request resolved: #29322
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests [ghstack-poisoned]
zou3519
added a commit
that referenced
this pull request
Nov 12, 2019
torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests ghstack-source-id: 3fa5177 Pull Request resolved: #29322
zdevito
pushed a commit
to zdevito/ATen
that referenced
this pull request
Nov 13, 2019
Summary: Pull Request resolved: pytorch/pytorch#29322 torch.equal checks if two tensors are equal in both size and values. For named tensors, it also checks that the names are exactly equal. There is an argument to be made for alternative semantics (check that the names *match*), but for an API that is called "equal" I would expect it to check equality on names as well. Test Plan: - new tests Differential Revision: D18453387 Pulled By: zou3519 fbshipit-source-id: d52bde4e3fdd7f331eef097a3b31d35c89c78049
Contributor
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:
torch.equal checks if two tensors are equal in both size and values. For
named tensors, it also checks that the names are exactly equal. There is
an argument to be made for alternative semantics (check that the names
match), but for an API that is called "equal" I would expect it to
check equality on names as well.
Test Plan:
Differential Revision: D18453387