Skip to content

[JIT] Add operator is, not, is not to script #13336

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
Closed
wants to merge 5 commits into from
Closed

[JIT] Add operator is, not, is not to script #13336

wants to merge 5 commits into from

Conversation

wanchaol
Copy link
Collaborator

As titled, this PR is a part of tasks to unblock exporting the standard library.

@wanchaol wanchaol added the oncall: jit Add this issue/PR to JIT oncall triage queue label Oct 30, 2018
wanchaol
wanchaol commented Oct 30, 2018
View reviewed changes
torch/csrc/jit/register_prim_ops.cpp Outdated
[](Node* node) {
return [=](Stack& stack) {
push(stack, !pop(stack).toInt());
return 0;
};
}),
Operator(
"aten::__not__(float self) -> bool",

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

driazati
driazati reviewed Oct 30, 2018
View reviewed changes
torch/csrc/jit/register_prim_ops.cpp Outdated
"aten::__not__(float self) -> bool",
[](Node* node) {
return [=](Stack& stack) {
push(stack, !pop(stack).toInt());

This comment was marked as off-topic.

Sign in to view

zdevito
zdevito approved these changes Oct 30, 2018
View reviewed changes
torch/csrc/jit/register_prim_ops.cpp Outdated
[](Node* node) {
return [=](Stack& stack) {
push(stack, !pop(stack).toInt());
return 0;
};
}),
Operator(
"aten::__not__(float self) -> bool",

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/script/lexer.h
@@ -53,6 +53,8 @@ namespace script {
_(TK_WHILE, "while", "while") \
_(TK_EXPR_STMT, "expression statement", "") \
_(TK_RETURN, "return", "return") \
_(TK_IS, "is", "is") \
_(TK_ISNOT, "is not", "is not") \

This comment was marked as off-topic.

Sign in to view

aten/src/ATen/core/ivalue.h Outdated
@@ -623,4 +626,27 @@ inline optional<T> IValue::toOptional() {
return this->to<T>();
}

inline bool IValue::isSameIdentity(IValue& rhs) {
// we choose to not use memcmp for payload check due to potenntial random padding characters on union type

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

@wanchaol wanchaol force-pushed the jitisnot branch 2 times, most recently from 8ce6fbb to a26efa8 Compare October 31, 2018 18:52
@wanchaol
Copy link
Collaborator Author

wanchaol commented Oct 31, 2018 •
edited
Loading

The error is happening because of py2 and py3 semantic difference for tuple:

in py2:

>>> (1, 1) is (1, 1)
False

while in py3:

>>> (1, 1) is (1, 1)
False

So is/(is not) op is different on tuples between py2 and py3. My current implementation is following python3 semantic, I am going to remove the tuple test case for now.

CC @zdevito

@wanchaol wanchaol force-pushed the jitisnot branch 2 times, most recently from 32460c5 to af3f147 Compare October 31, 2018 20:43
@zdevito
Copy link
Contributor

zdevito commented Oct 31, 2018

is has a bunch of undefined semantics in Python. We should not try too hard to match the exact behavior of the CPython implementation, because it can cause us to need to support its quirks indefinitely. I think for our current data types this behavior should be simple and within spec:

  1. If it is a reference type (i.e. is_intrusive_ptr), then is is True when the pointed-to object is the same.
  2. None is None, False is False, and True is True are all true (I think the last two cases are technically unneeded as far as spec goes).
  3. False for all other comparisons.

facebook-github-bot
facebook-github-bot reviewed Nov 1, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

facebook-github-bot
facebook-github-bot reviewed Nov 1, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wanchaol wanchaol deleted the jitisnot branch November 1, 2018 23:57
zdevito pushed a commit to zdevito/ATen that referenced this pull request Nov 2, 2018
@wanchaol @facebook-github-bot
Add operator is, not, is not to script (#13336)
f30f39e 
Summary:
As titled, this PR is a part of tasks to unblock exporting the standard library.
Pull Request resolved: pytorch/pytorch#13336

Differential Revision: D12888912

Pulled By: wanchaol

fbshipit-source-id: 6213a17a75a593ae45999994fd9562f29b7d42df
@ezyang ezyang added the merged label Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zdevito zdevito zdevito approved these changes

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

+1 more reviewer

@driazati driazati driazati left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wanchaol @zdevito @facebook-github-bot @driazati @ezyang