Skip to content

Return NotImplemented from tensor arithmetic operators #26507

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
peterbell10 wants to merge 1 commit into from
Closed

Return NotImplemented from tensor arithmetic operators #26507

peterbell10 wants to merge 1 commit into from

Conversation

@peterbell10
Copy link
Collaborator

@peterbell10 peterbell10 commented Sep 19, 2019

Fixes #26333

This effectively rewrites all infix arithmetic operators op as:

def __op__(self, other):
    try:
        return self.op(other)
    except TypeError:
        return NotImplemented

Where TypeError is raised from the argument parser when other is not a Tensor. This should be okay, so long as TypeError isn't raised by the function for any other reasons. I couldn't find any examples where this was an issue.

@peterbell10 peterbell10 requested a review from ezyang September 19, 2019 22:25
@pytorchbot pytorchbot added module: internals Related to internal abstractions in c10 and ATen module: pybind Related to our Python bindings / interactions with other Python libraries labels Sep 19, 2019
@peterbell10 peterbell10 force-pushed the peterbell10/operator-NotImplemented branch from 4304869 to 720f0ab Compare September 20, 2019 12:39
ezyang
ezyang approved these changes Sep 20, 2019
View reviewed changes
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

Nifty!

@ezyang
Copy link
Contributor

ezyang commented Sep 20, 2019

@pytorchbot rebase this please

@ezyang
Copy link
Contributor

ezyang commented Sep 20, 2019

Waiting on CI

@pytorchbot
Copy link
Collaborator

pytorchbot commented Sep 20, 2019

Sorry, I can't merge this because there are conflicts. To merge this yourself, run the commands below:

git fetch origin master
git fetch [email protected]:Quansight/pytorch.git peterbell10/operator-NotImplemented
git checkout FETCH_HEAD
git merge origin/master
git push [email protected]:Quansight/pytorch.git HEAD:peterbell10/operator-NotImplemented

(To learn more about this bot, see Bot commands.)

@peterbell10 peterbell10 force-pushed the peterbell10/operator-NotImplemented branch from 720f0ab to 2024e0c Compare September 20, 2019 18:30
@ezyang
Copy link
Contributor

ezyang commented Sep 20, 2019

@pytorchbot rebase this please

@pytorchbot
Copy link
Collaborator

pytorchbot commented Sep 20, 2019

Sorry, I can't merge this because there are conflicts. To merge this yourself, run the commands below:

git fetch origin master
git fetch [email protected]:Quansight/pytorch.git peterbell10/operator-NotImplemented
git checkout FETCH_HEAD
git merge origin/master
git push [email protected]:Quansight/pytorch.git HEAD:peterbell10/operator-NotImplemented

(To learn more about this bot, see Bot commands.)

@ezyang
Copy link
Contributor

ezyang commented Sep 20, 2019

Gosh, this PR conflicts a lot for some reason.

@ezyang
Copy link
Contributor

ezyang commented Sep 20, 2019

Oh, you didn't grant maintainers rights to update the PR, so the bot can't update it. Could you please rebase again to clear up the errors?

@peterbell10 peterbell10 force-pushed the peterbell10/operator-NotImplemented branch from 2024e0c to ec97fe6 Compare September 20, 2019 21:29
@peterbell10
Copy link
Collaborator Author

peterbell10 commented Sep 20, 2019

Oh, you didn't grant maintainers rights to update the PR, so the bot can't update it.

That's odd, "Allow edits from maintainers." is shown a selected on my screen. Could it be an issue with submitting from the Quansight repo?

cc @rgommers

@rgommers
Copy link
Collaborator

rgommers commented Sep 21, 2019

That's odd, "Allow edits from maintainers." is shown a selected on my screen. Could it be an issue with submitting from the Quansight repo?

Weirdly enough, I think it is. Just tried on PR 25629 and that failed as well.

Our repository settings are a bit unusual, I'll look into it.

Tried adding pytorchbot as a collaborator to fix it, but that requires it to accept an invitation:

image

I'll try to look for a more structural fix, because if it is repository settings then it will happen on other repos as well.

@ezyang
Copy link
Contributor

ezyang commented Sep 23, 2019

I had pytorchbot accept the invitation.

@ezyang
Copy link
Contributor

ezyang commented Sep 23, 2019

@pytorchbot rebase this please

@pytorchbot
Copy link
Collaborator

pytorchbot commented Sep 23, 2019

Sorry, I can't merge this because there are conflicts. To merge this yourself, run the commands below:

git fetch origin master
git fetch [email protected]:Quansight/pytorch.git peterbell10/operator-NotImplemented
git checkout FETCH_HEAD
git merge origin/master
git push [email protected]:Quansight/pytorch.git HEAD:peterbell10/operator-NotImplemented

(To learn more about this bot, see Bot commands.)

@ezyang
Copy link
Contributor

ezyang commented Sep 23, 2019

Didn't work :/

@rgommers
Copy link
Collaborator

rgommers commented Sep 23, 2019

Hmm, then I have no idea. Will have to do some trial and error on a dummy repo. @peterbell10 for next PRs I suggest to open them from your own fork until we figure this out.

@peterbell10 peterbell10 force-pushed the peterbell10/operator-NotImplemented branch from ec97fe6 to 7268b60 Compare September 23, 2019 15:16
facebook-github-bot
facebook-github-bot reviewed Sep 30, 2019
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.

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 30, 2019

@ezyang merged this pull request in 585a597.

@peterbell10 peterbell10 deleted the peterbell10/operator-NotImplemented branch September 30, 2019 19:45
@gchanan
Copy link
Contributor

gchanan commented Oct 1, 2019

should we be returning NotImplemented for the inplace functions? That doesn't seem correct.

Also, this doesn't work fork for functions that are written in native_functions.yaml and aren't explicitly bound.

@gchanan gchanan mentioned this pull request Oct 1, 2019
Closed
@ezyang
Copy link
Contributor

ezyang commented Oct 2, 2019

The docs specify that it is valid for inplace functions too:

Special value which should be returned by the binary special methods (e.g. eq(), lt(), add(), rsub(), etc.) to indicate that the operation is not implemented with respect to the other type; may be returned by the in-place binary special methods (e.g. imul(), iand(), etc.) for the same purpose. Its truth value is true.

Rereviewing this diff, I regret not asking for tests though...

@peterbell10 peterbell10 mentioned this pull request Oct 5, 2019
Closed
facebook-github-bot pushed a commit that referenced this pull request Oct 28, 2019
@peterbell10 @facebook-github-bot
Return NotImplemented from all binary math ops (#27423)
f33813d 
Summary:
Fixes #26333

Fixes the operators missed in #26507 and includes a test for all operators.
Pull Request resolved: #27423

Differential Revision: D17835390

Pulled By: ezyang

fbshipit-source-id: 7a1351c7ccc8ad11454dbaa00d3701dcee4f06a8
pdlive215 pushed a commit to pdlive215/pytorch that referenced this pull request Nov 27, 2019
@peterbell10 @pdlive215
Return NotImplemented from tensor arithmetic operators (pytorch#26507)
aea1a95 
Summary:
Fixes pytorch#26333

This effectively rewrites all infix arithmetic operators *op* as:
```python
def __op__(self, other):
    try:
        return self.op(other)
    except TypeError:
        return NotImplemented
```

Where `TypeError` is raised from the argument parser when `other` is not a `Tensor`. This should be okay, so long as `TypeError` isn't raised by the function for any other reasons. I couldn't find any examples where this was an issue.
Pull Request resolved: pytorch#26507

Differential Revision: D17669097

Pulled By: ezyang

fbshipit-source-id: 2c1a1087057c9298915d713d3fea7d682d014c72
thiagocrepaldi pushed a commit to thiagocrepaldi/pytorch that referenced this pull request Feb 4, 2020
@peterbell10
Return NotImplemented from tensor arithmetic operators (pytorch#26507)
c5c7149 
Summary:
Fixes pytorch#26333

This effectively rewrites all infix arithmetic operators *op* as:
```python
def __op__(self, other):
    try:
        return self.op(other)
    except TypeError:
        return NotImplemented
```

Where `TypeError` is raised from the argument parser when `other` is not a `Tensor`. This should be okay, so long as `TypeError` isn't raised by the function for any other reasons. I couldn't find any examples where this was an issue.
Pull Request resolved: pytorch#26507

Differential Revision: D17669097

Pulled By: ezyang

fbshipit-source-id: 2c1a1087057c9298915d713d3fea7d682d014c72
@albanD albanD mentioned this pull request Mar 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@ezyang ezyang ezyang approved these changes

Assignees

No one assigned

Labels

Merged module: internals Related to internal abstractions in c10 and ATen module: pybind Related to our Python bindings / interactions with other Python libraries open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

torch.Tensor reverse operators (__rmul__) should return NotImplemented for unsupported types

7 participants

@peterbell10 @ezyang @pytorchbot @rgommers @facebook-github-bot @gchanan @mruberry