Return NotImplemented from all binary math ops #27423

peterbell10 · 2019-10-05T14:44:23Z

Fixes #26333

Fixes the operators missed in #26507 and includes a test for all operators.

ezyang · 2019-10-07T13:36:30Z

torch/tensor.py

Can you remind me why we don't implement the wrapping here directly in C++?

It doesn't make sense to return NotImplemented from Tensor.eq which is what this is wrapping.

ezyang · 2019-10-07T13:39:01Z

test/test_torch.py

cc @mruberry (any thoughts here?)

This is well-written and makes good use of the existing test infrastructure. It may be test overkill to add hundreds of tests for this behavior, but as long as these take <1s to run it's fine. (You can check the timestamps in the CI.)

test/test_torch.py

ezyang

OK. I'll leave a little time for mruberry to take a look and then merge.

mruberry · 2019-10-07T17:24:56Z

test/test_torch.py

Not "TestDevicePrecision" per below. You should acquire the class name from cls.

ezyang · 2019-10-07T23:31:05Z

I strongly suspect the onnx test failures are real. Probbabbbly just accepting the changes should be fine

peterbell10 · 2019-10-08T20:21:41Z

I'm not familiar with ONNX, does it depend on the parameter names in the function signature? If that's the case then could this error be caused by _wrap_type_error_to_not_implemented giving a function that takes *args?

Oct 07 20:00:24 E   AssertionError: 'ir_version: 4\nproducer_name: "pytorch"\nproducer_version: "1.3"\ngraph {\n  no [truncated]... != u'ir_version: 4\nproducer_name: "pytorch"\nproducer_version: "1.3"\ngraph {\n  n [truncated]...
Oct 07 20:00:24 E     ir_version: 4
Oct 07 20:00:24 E     producer_name: "pytorch"
Oct 07 20:00:24 E     producer_version: "1.3"
Oct 07 20:00:24 E     graph {
Oct 07 20:00:24 E       node {
Oct 07 20:00:24 E   -     input: "0"
Oct 07 20:00:24 E   ?             ^
Oct 07 20:00:24 E   +     input: "x"
Oct 07 20:00:24 E   ?             ^
Oct 07 20:00:24 E   -     input: "1"
Oct 07 20:00:24 E   ?             ^
Oct 07 20:00:24 E   +     input: "y"
Oct 07 20:00:24 E   ?             ^

mruberry · 2019-10-08T20:27:56Z

@houseroad for ONNX question

ezyang · 2019-10-09T15:31:57Z

@peterbell10 Yes, I don't exactly remember how argument name inference is done, but it certainly seems plausible that decorator is thwarting (or un-thwarting, perhaps) the name inference. The names are all advisory anyway; if the graphs are alpha-equivalent either is fine.

peterbell10 · 2019-10-09T15:45:54Z

The names are all advisory anyway; if the graphs are alpha-equivalent either is fine.

Okay, in that case I've just updated the test cases.

ezyang · 2019-10-09T18:24:56Z

@pytorchbot rebase this please

facebook-github-bot

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

ezyang · 2019-10-17T15:50:36Z

Sorry, it looks like this merge conflicts now :(

ezyang · 2019-10-22T20:45:02Z

Still failing tests

Oct 17 17:05:59 ____________________________ TestOperators.test_ne _____________________________
Oct 17 17:05:59 
Oct 17 17:05:59 self = <test_operators.TestOperators testMethod=test_ne>
Oct 17 17:05:59 
Oct 17 17:05:59     def test_ne(self):
Oct 17 17:05:59         x = torch.randn(1, 2, 3, 1, requires_grad=False).int()
Oct 17 17:05:59         y = torch.randn(1, 4, requires_grad=False).int()
Oct 17 17:05:59 >       self.assertONNX(lambda x, y: torch.ne(x, y), (x, y))
Oct 17 17:05:59 
Oct 17 17:05:59 test/onnx/test_operators.py:633: 
Oct 17 17:05:59 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
Oct 17 17:05:59 test/onnx/test_operators.py:67: in assertONNX
Oct 17 17:05:59     self.assertExpected(onnx_model_pbtxt, subname)
Oct 17 17:05:59 test/common_utils.py:944: in assertExpected
Oct 17 17:05:59     self.assertMultiLineEqual(expected, s)
Oct 17 17:05:59 E   AssertionError: 'ir_version: 4\nproducer_name: "pytorch"\nproducer_version: "1.3"\ngraph {\n  no [truncated]... != u'ir_version: 4\nproducer_name: "pytorch"\nproducer_version: "1.3"\ngraph {\n  n [truncated]...
Oct 17 17:05:59 E     ir_version: 4
Oct 17 17:05:59 E     producer_name: "pytorch"
Oct 17 17:05:59 E     producer_version: "1.3"
Oct 17 17:05:59 E     graph {
Oct 17 17:05:59 E       node {
Oct 17 17:05:59 E   -     input: "x"
Oct 17 17:05:59 E   ?             ^
Oct 17 17:05:59 E   +     input: "0"
Oct 17 17:05:59 E   ?             ^
Oct 17 17:05:59 E   -     input: "y"
Oct 17 17:05:59 E   ?             ^
Oct 17 17:05:59 E   +     input: "1"
Oct 17 17:05:59 E   ?             ^
Oct 17 17:05:59 E         output: "2"
Oct 17 17:05:59 E         op_type: "Equal"
Oct 17 17:05:59 E       }
Oct 17 17:05:59 E       node {
Oct 17 17:05:59 E         input: "2"
Oct 17 17:05:59 E         output: "3"
Oct 17 17:05:59 E         op_type: "Not"
Oct 17 17:05:59 E       }
Oct 17 17:05:59 E       name: "torch-jit-export"
Oct 17 17:05:59 E       input {
Oct 17 17:05:59 E   -     name: "x"
Oct 17 17:05:59 E   ?            ^
Oct 17 17:05:59 E   +     name: "0"
Oct 17 17:05:59 E   ?            ^
Oct 17 17:05:59 E         type {
Oct 17 17:05:59 E           tensor_type {
Oct 17 17:05:59 E             elem_type: 6
Oct 17 17:05:59 E             shape {
Oct 17 17:05:59 E               dim {
Oct 17 17:05:59 E                 dim_value: 1
Oct 17 17:05:59 E               }
Oct 17 17:05:59 E               dim {
Oct 17 17:05:59 E                 dim_value: 2
Oct 17 17:05:59 E               }
Oct 17 17:05:59 E               dim {
Oct 17 17:05:59 E                 dim_value: 3
Oct 17 17:05:59 E               }
Oct 17 17:05:59 E               dim {
Oct 17 17:05:59 E                 dim_value: 1
Oct 17 17:05:59 E               }
Oct 17 17:05:59 E             }
Oct 17 17:05:59 E           }
Oct 17 17:05:59 E         }
Oct 17 17:05:59 E       }
Oct 17 17:05:59 E       input {
Oct 17 17:05:59 E   -     name: "y"
Oct 17 17:05:59 E   ?            ^
Oct 17 17:05:59 E   +     name: "1"
Oct 17 17:05:59 E   ?            ^
Oct 17 17:05:59 E         type {
Oct 17 17:05:59 E           tensor_type {
Oct 17 17:05:59 E             elem_type: 6
Oct 17 17:05:59 E             shape {
Oct 17 17:05:59 E               dim {
Oct 17 17:05:59 E                 dim_value: 1
Oct 17 17:05:59 E               }
Oct 17 17:05:59 E               dim {
Oct 17 17:05:59 E                 dim_value: 4
Oct 17 17:05:59 E               }
Oct 17 17:05:59 E             }
Oct 17 17:05:59 E           }
Oct 17 17:05:59 E         }
Oct 17 17:05:59 E       }
Oct 17 17:05:59 E       output {
Oct 17 17:05:59 E         name: "3"
Oct 17 17:05:59 E         type {
Oct 17 17:05:59 E           tensor_type {
Oct 17 17:05:59 E             elem_type: 9
Oct 17 17:05:59 E             shape {
Oct 17 17:05:59 E               dim {
Oct 17 17:05:59 E                 dim_value: 1
Oct 17 17:05:59 E               }
Oct 17 17:05:59 E               dim {
Oct 17 17:05:59 E                 dim_value: 2
Oct 17 17:05:59 E               }
Oct 17 17:05:59 E               dim {
Oct 17 17:06:00 E                 dim_value: 3
Oct 17 17:06:00 E               }
Oct 17 17:06:00 E               dim {
Oct 17 17:06:00 E                 dim_value: 4
Oct 17 17:06:00 E               }
Oct 17 17:06:00 E             }
Oct 17 17:06:00 E           }
Oct 17 17:06:00 E         }
Oct 17 17:06:00 E       }
Oct 17 17:06:00 E     }
Oct 17 17:06:00 E     opset_import {
Oct 17 17:06:00 E       version: 9
Oct 17 17:06:00 E     }
Oct 17 17:06:00 =============================== warnings summary ===============================

peterbell10 · 2019-10-22T20:53:15Z

Should be fixed now.

ezyang · 2019-10-23T18:09:28Z

@pytorchbot rebase this please

facebook-github-bot

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

facebook-github-bot · 2019-10-29T04:11:25Z

@ezyang merged this pull request in f33813d.

peterbell10 added the open source label Oct 5, 2019

peterbell10 requested a review from ezyang October 5, 2019 14:44

pytorchbot added module: internals Related to internal abstractions in c10 and ATen module: operators module: pybind Related to our Python bindings / interactions with other Python libraries labels Oct 5, 2019

ezyang reviewed Oct 7, 2019

View reviewed changes

test/test_torch.py Outdated Show resolved Hide resolved

ezyang approved these changes Oct 7, 2019

View reviewed changes

mruberry reviewed Oct 7, 2019

View reviewed changes

test/test_torch.py Outdated

Copy link

Collaborator

mruberry Oct 7, 2019

Choose a reason for hiding this comment

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

Not "TestDevicePrecision" per below. You should acquire the class name from cls.

pytorchbot added the module: onnx Related to torch.onnx label Oct 9, 2019

facebook-github-bot reviewed Oct 9, 2019

View reviewed changes

peterbell10 added 5 commits October 17, 2019 17:05

Return NotImplemented from all binary math ops

c440683

Test binary ops return NotImplemented

f530276

Workaround python 2 functools.wraps poor support for methods

de4001e

Run NotImplemented tests on cpu and avoid duplicating calls

712c866

Fix test name in assert message

afcb5e3

peterbell10 force-pushed the binary-math-ops branch from 6ea268b to 15bf25b Compare October 17, 2019 16:09

Update parameter names in test/onnx/expect

a49c663

peterbell10 force-pushed the binary-math-ops branch from 15bf25b to a49c663 Compare October 22, 2019 20:51

Merge remote-tracking branch 'origin/master' into binary-math-ops

a84644f

facebook-github-bot reviewed Oct 25, 2019

View reviewed changes

facebook-github-bot closed this in f33813d Oct 28, 2019

facebook-github-bot added the merged label Oct 29, 2019

mruberry added the Merged label Oct 28, 2020

peterbell10 mentioned this pull request Nov 16, 2020

move Tensor comparisons back to C #48018

Closed

Return NotImplemented from all binary math ops #27423

Return NotImplemented from all binary math ops #27423

Uh oh!

Conversation

peterbell10 commented Oct 5, 2019

Uh oh!

ezyang Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

peterbell10 Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

ezyang Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

mruberry Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ezyang left a comment

Choose a reason for hiding this comment

Uh oh!

mruberry Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

ezyang commented Oct 7, 2019

Uh oh!

peterbell10 commented Oct 8, 2019

Uh oh!

mruberry commented Oct 8, 2019

Uh oh!

ezyang commented Oct 9, 2019

Uh oh!

peterbell10 commented Oct 9, 2019

Uh oh!

ezyang commented Oct 9, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented Oct 17, 2019

Uh oh!

ezyang commented Oct 22, 2019

Uh oh!

peterbell10 commented Oct 22, 2019

Uh oh!

ezyang commented Oct 23, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants