Let logical_xor support non-bool tensors (again) #27248

xuhdev · 2019-10-02T22:39:31Z

f362a5a reverted
5ca612b due to build time conerns (also
see #25254). Now we come back to this by reusing the underlying code in
comparison operators: Logical operators on non-bool variables are
essentially comparison operators that semantically output bool
values. Compared with the previous implementation, we compromise by
always applying XOR on the same input type, while output can be either
the input type or the bool type.

ifedan · 2019-10-03T20:14:52Z

There are some issues with tests

xuhdev · 2019-10-03T20:33:11Z

@pytorchbot retest this please

ifedan · 2019-10-08T00:43:32Z

We do not support Byte output for logical operation any more. So some test are failing.

xuhdev · 2019-10-08T17:53:18Z

We do not support Byte output for logical operation any more. So some test are failing.

Even if we do out=a byte tensor? I thought the output is bool only if there is no output type specified (e.g., a > b instead of a.gt(b, out=c)).

xuhdev · 2019-10-08T18:18:22Z

@ifedan I changed the test

ifedan · 2019-10-10T17:18:07Z

You have some failures in tests, because for inplace operation a.op_(b) we expect that a and b has same dtype, otherwise it's possible to get an overflow(Example: 'a' is uint8, 'b' is float32. 'a' will be promoted to float32 and the result will be also float32. Then it will be casted back to uint8 so potential for overflow). So we check that both dtypes of a and b are same

xuhdev · 2019-10-11T17:08:27Z

@ifedan I've updated the overtest again. Thanks!

ifedan · 2019-10-11T23:20:40Z

logical_xor_cpu is not implemented for 'Half'

xuhdev · 2019-10-11T23:50:16Z

Updated again

xuhdev · 2019-10-13T02:13:47Z

I skipped the test case where the dtype of either operand is bfloat16, as it seems like type promotion hasn't been handled for it yet for comparison ops. Probably it is more appropriate to fix in another PR.

In [2]: a=torch.tensor([0, 0], device='cpu', dtype=torch.int)

In [3]: b=torch.tensor([0, 0], device='cpu', dtype=torch.bfloat16)

In [4]: a == b
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-4-eeadcaeab874> in <module>
----> 1 a == b

RuntimeError: dtype != ScalarType::Undefined INTERNAL ASSERT FAILED at /home/hong/xusrc/pytorch/aten/src/ATen/native/TensorIterator.cpp:97, please report a bug to PyTorch.

xuhdev · 2019-10-14T20:10:11Z

@pytorchbot rebase this please

f362a5a reverted 5ca612b due to build time conerns (also see pytorch#25254). Now we come back to this by reusing the underlying code in comparison operators: Logical operators on non-bool variables are essentially comparison operators that semantically output bool values. Compared with the previous implementation, we compromise by always applying XOR on the same input type, while output can be either the input type or the bool type.

xuhdev · 2019-10-15T03:29:36Z

@pytorchbot merge this please

facebook-github-bot

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

Summary: f362a5a04b3708355b08e5c1285e46ca1b537ad6 reverted 5ca612b55ec1205f98e6bc5d5e64b1bf35f3b3cd due to build time conerns (also see pytorch/pytorch#25254). Now we come back to this by reusing the underlying code in comparison operators: Logical operators on non-bool variables are essentially comparison operators that semantically output bool values. Compared with the previous implementation, we compromise by always applying XOR on the same input type, while output can be either the input type or the bool type. Pull Request resolved: pytorch/pytorch#27248 Differential Revision: D17929356 Pulled By: ezyang fbshipit-source-id: dbac08c7614b36f05d24c69104fee9df9ca523d5

gchanan · 2019-10-15T19:05:40Z

aten/src/ATen/native/BinaryOps.cpp

 Tensor& comparison_op_out(Tensor& result, const Tensor& self, const Tensor& other, Stub& stub) {
  TORCH_CHECK(result.scalar_type() == kBool,
-              "The output tensor of lt must be a bool, but was ", result.scalar_type());
+              "The output tensor of a comparison or logical op must be a bool, but was ", result.scalar_type());


I'm confused -- the documentation shows the output being uint8, but this error messages says it can be a bool. What's up?

gchanan · 2019-10-15T19:09:05Z

test/test_torch.py

+            for other_dtype in torch.testing.get_all_dtypes():
+                # Skip bfloat16 on CUDA
+                if device != 'cpu' and torch.bfloat16 in (dtype, other_dtype):
+                    continue


we generally don't test like this -- it's not good because we will eventually enable this op but we won't realize we need to enable testing. It's much better to assert that the test throws, so when we add support we are forced to test it.

@gchanan

Following up pytorch#27248 per suggestion by @gchanan

facebook-github-bot · 2019-10-15T23:46:14Z

@ezyang merged this pull request in e6a7140.

Summary: Following up #27248. per suggestion by gchanan Pull Request resolved: #28031 Differential Revision: D17962226 Pulled By: gchanan fbshipit-source-id: 788e4e1fc78b1cfc7915aedaa10c8656b19edc4d

Summary: f362a5a reverted 5ca612b due to build time conerns (also see pytorch#25254). Now we come back to this by reusing the underlying code in comparison operators: Logical operators on non-bool variables are essentially comparison operators that semantically output bool values. Compared with the previous implementation, we compromise by always applying XOR on the same input type, while output can be either the input type or the bool type. Pull Request resolved: pytorch#27248 Differential Revision: D17929356 Pulled By: ezyang fbshipit-source-id: dbac08c7614b36f05d24c69104fee9df9ca523d5

Summary: Following up pytorch#27248. per suggestion by gchanan Pull Request resolved: pytorch#28031 Differential Revision: D17962226 Pulled By: gchanan fbshipit-source-id: 788e4e1fc78b1cfc7915aedaa10c8656b19edc4d

xuhdev requested review from gchanan and ifedan October 2, 2019 22:39

pytorchbot added module: cpu CPU specific problem (e.g., perf, algorithm) module: cuda Related to torch.cuda, and CUDA support in general module: docs Related to our documentation, both in docs/ and docblocks module: operators labels Oct 2, 2019

ezyang added the open source label Oct 2, 2019

xuhdev force-pushed the logical-xor branch from 7c77a7d to ca7287c Compare October 3, 2019 20:29

xuhdev force-pushed the logical-xor branch from ca7287c to d01866d Compare October 3, 2019 21:03

jerryzh168 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 5, 2019

xuhdev force-pushed the logical-xor branch from d01866d to 40c8762 Compare October 8, 2019 17:46

xuhdev force-pushed the logical-xor branch from 40c8762 to 8a372c2 Compare October 8, 2019 18:16

xuhdev force-pushed the logical-xor branch from 8a372c2 to ffb0c7f Compare October 11, 2019 17:07

xuhdev force-pushed the logical-xor branch from ffb0c7f to 9ce03d2 Compare October 11, 2019 23:49

xuhdev force-pushed the logical-xor branch 2 times, most recently from ca49c76 to 5ea8a22 Compare October 13, 2019 02:11

xuhdev force-pushed the logical-xor branch 4 times, most recently from 893ab03 to b9ae14f Compare October 14, 2019 07:28

xuhdev force-pushed the logical-xor branch from b9ae14f to 9ff8f05 Compare October 14, 2019 17:55

xuhdev force-pushed the logical-xor branch from 9ff8f05 to eb9f2cd Compare October 14, 2019 21:02

ifedan approved these changes Oct 15, 2019

View reviewed changes

pytorchbot added the merge-this-please Was marked for merge with @pytorchbot merge this please label Oct 15, 2019

facebook-github-bot reviewed Oct 15, 2019

View reviewed changes

facebook-github-bot closed this in e6a7140 Oct 15, 2019

gchanan reviewed Oct 15, 2019

View reviewed changes

xuhdev deleted the logical-xor branch October 15, 2019 19:08

gchanan reviewed Oct 15, 2019

View reviewed changes

xuhdev added a commit to xuhdev/pytorch that referenced this pull request Oct 15, 2019

Improve the doc and test of logical_xor

d39f2f7

Following up pytorch#27248 per suggestion by @gchanan

xuhdev mentioned this pull request Oct 15, 2019

Improve the doc and test of logical_xor #28031

Closed

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

xuhdev mentioned this pull request Oct 24, 2019

Add logical_and and logical_or #28162

Closed

xuhdev mentioned this pull request Jun 18, 2020

Let exp support complex types on CUDA and enable device/dtype in complex tests #39087

Closed

mruberry added the Merged label Oct 28, 2020

Let logical_xor support non-bool tensors (again) #27248

Let logical_xor support non-bool tensors (again) #27248

Uh oh!

Conversation

xuhdev commented Oct 2, 2019

Uh oh!

ifedan commented Oct 3, 2019

Uh oh!

xuhdev commented Oct 3, 2019

Uh oh!

ifedan commented Oct 8, 2019

Uh oh!

xuhdev commented Oct 8, 2019

Uh oh!

xuhdev commented Oct 8, 2019

Uh oh!

ifedan commented Oct 10, 2019

Uh oh!

xuhdev commented Oct 11, 2019

Uh oh!

ifedan commented Oct 11, 2019

Uh oh!

xuhdev commented Oct 11, 2019

Uh oh!

xuhdev commented Oct 13, 2019

Uh oh!

xuhdev commented Oct 14, 2019

Uh oh!

xuhdev commented Oct 15, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gchanan Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

gchanan Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants