is_numpy_scalar should also consider bool and complex types #43644

xuhdev · 2020-08-26T19:12:21Z

Before this PR,

import torch
import numpy as np

a = torch.tensor([1, 2], dtype=torch.bool)
c = np.array([1, 2], dtype=np.bool)
print(a[0] == c[0])

a = torch.tensor([1, 2], dtype=torch.complex64)
c = np.array([1, 2], dtype=np.complex64)
print(a[0] == c[0])

 # This case is still broken
a = torch.tensor([1 + 1j, 2 + 2j], dtype=torch.complex64)
c = np.array([1 + 1j, 2 + 2j], dtype=np.complex64)
print(a[0] == c[0])

outputs

False
False
False

After this PR, it outputs:

tensor(True)
/home/user/src/pytorch/torch/tensor.py:25: ComplexWarning: Casting complex values to real discards the imaginary part return f(*args, **kwargs)
tensor(True)
tensor(False)

Related issue: #43579

cc @anjali411 @mruberry

xuhdev · 2020-08-26T19:13:37Z

I couldn't find any Python exposure of is_numpy_scalar. Does anyone know where this is tested?

mruberry · 2020-08-26T19:20:26Z

I couldn't find any Python exposure of is_numpy_scalar. Does anyone know where this is tested?

You'd have to test it in C++ if you wanted to test it directly. Instead, however, you can probably test the desired effect (testing is_numpy_scalar indirectly), by adding or extending a test of our NumPy interop. Those tests are in test_torch (although I've been meaning to pull them out into their own test suite to make sifting through them easier). If you wanted to be very thorough you could even write a new test that creates torch tensors and their corresponding NumPy arrays and verifies they compare as equal for all dtypes.

xuhdev · 2020-08-27T19:41:56Z

@mruberry I've added test now

codecov · 2020-08-27T22:31:18Z

Codecov Report

Merging #43644 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #43644   +/-   ##
=======================================
  Coverage   69.28%   69.29%           
=======================================
  Files         379      379           
  Lines       47035    47035           
=======================================
+ Hits        32590    32591    +1     
+ Misses      14445    14444    -1

Impacted Files	Coverage Δ
torch/testing/_internal/expecttest.py	`78.57% <0.00%> (+1.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7035cd0...fc4728d. Read the comment docs.

test/test_torch.py

mruberry · 2020-08-28T21:57:05Z

test/test_torch.py

Currently the complex tensors won't actually have any complex values and there's only a single 1D tensor. What about extending the test slightly to support complex values and multiple tensor sizes? It could do something like this:

is_complex = dtype in (torch.cfloat, torch.cdouble) if is_complex: tensors = (...) else: tensors = ( torch.tensor(5, dtype=dtype, device=device), torch.tensor([0, 1, 2], dtype=dtype, device=device), torch.tensor([[1, 2], [3, 4]], dtype=dtype, device=device), ) for tensor in tensors: np_array = tensor.cpu().numpy() for t, a in product((tensor[0], tensor[0].item()), (np_array[0], np_array[0].item()): self.assertEqual(t, a) if not is_complex: self.assertTrue(t == a) if is_complex: self.assertTrue(t[0] == np_array[0].item()) self.assertTrue(t[0].item() == np_array[0]) self.assertFalse(t[0] == np_array[0])

Sounds good

Per the following discussion, if you follow this pattern the later conditionals will probably need to check dtype is torch.cfloat instead of the is_complex boolean.

What do you mean?

Won't self.assertFalse(t[0] == np_array[0]) be true only for cfloat, not for cdouble?

Yes, thanks. Anyway, I have structured the test a bit differently.

test/test_torch.py

mruberry · 2020-08-28T21:57:59Z

Hey @xuhdev! This looks really cool. Just have one question about testing for you to review. Looking forward to hearing your thoughts!

anjali411 · 2020-08-31T14:04:37Z

test/test_torch.py

@mruberry do we have any function similar to _make_tensors in TestTorchDeviceType?

Recap of offline discussion: #43451 is adding a make_tensor method to common_utils.py.

facebook-github-bot

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

facebook-github-bot

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

anjali411 · 2020-09-01T18:23:13Z

@xuhdev can you rebase this PR again? phabricator is complaining since another PR landed touching files edited in this PR

Before this PR, ```python import torch import numpy as np a = torch.tensor([1, 2], dtype=torch.bool) c = np.array([1, 2], dtype=np.bool) print(a[0] == c[0]) a = torch.tensor([1, 2], dtype=torch.complex64) c = np.array([1, 2], dtype=np.complex64) print(a[0] == c[0]) # This case is still broken a = torch.tensor([1 + 1j, 2 + 2j], dtype=torch.complex64) c = np.array([1 + 1j, 2 + 2j], dtype=np.complex64) print(a[0] == c[0]) ``` outputs ``` False False False ``` After this PR, it outputs: ``` tensor(True) /home/hong/xusrc/pytorch/torch/tensor.py:25: ComplexWarning: Casting complex values to real discards the imaginary part return f(*args, **kwargs) tensor(True) tensor(False) ``` Related issue: pytorch#43579

xuhdev · 2020-09-01T18:38:20Z

@anjali411 Done!

facebook-github-bot

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

facebook-github-bot · 2020-09-02T16:13:48Z

@anjali411 merged this pull request in 4bb5d33.

xuhdev mentioned this pull request Aug 26, 2020

For the same complex dtype and same value, comparing a PyTorch tensor with a NumPy array results in False #43579

Open

pytorchbot added the open source label Aug 26, 2020

mruberry added module: complex Related to complex number support in PyTorch triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Aug 26, 2020

mruberry requested review from anjali411 and mruberry August 26, 2020 19:20

mruberry added the module: numpy Related to numpy support, and also numpy compatibility of our operators label Aug 26, 2020

xuhdev force-pushed the is-numpy-scalar branch from 68ce73e to d7e81e5 Compare August 27, 2020 19:41

xuhdev force-pushed the is-numpy-scalar branch from d7e81e5 to 052d19a Compare August 27, 2020 19:42

xuhdev requested a review from nairbv August 27, 2020 23:09

mruberry reviewed Aug 28, 2020

View reviewed changes

test/test_torch.py Outdated Show resolved Hide resolved

mruberry reviewed Aug 28, 2020

View reviewed changes

test/test_torch.py Outdated Show resolved Hide resolved

mruberry reviewed Aug 28, 2020

View reviewed changes

test/test_torch.py Outdated Show resolved Hide resolved

xuhdev force-pushed the is-numpy-scalar branch from 052d19a to 0a74895 Compare August 28, 2020 23:08

xuhdev requested a review from mruberry August 28, 2020 23:08

xuhdev force-pushed the is-numpy-scalar branch 2 times, most recently from 944b26f to 5faeaa0 Compare August 29, 2020 01:07

anjali411 reviewed Aug 31, 2020

View reviewed changes

anjali411 approved these changes Aug 31, 2020

View reviewed changes

facebook-github-bot reviewed Aug 31, 2020

View reviewed changes

facebook-github-bot reviewed Sep 1, 2020

View reviewed changes

xuhdev added 2 commits September 1, 2020 11:37

Add test

fc4728d

xuhdev force-pushed the is-numpy-scalar branch from 5faeaa0 to fc4728d Compare September 1, 2020 18:38

facebook-github-bot reviewed Sep 1, 2020

View reviewed changes

facebook-github-bot closed this in 4bb5d33 Sep 2, 2020

facebook-github-bot added the merged label Sep 2, 2020

xuhdev deleted the is-numpy-scalar branch September 3, 2020 09:04

mruberry added the Merged label Oct 28, 2020

is_numpy_scalar should also consider bool and complex types #43644

is_numpy_scalar should also consider bool and complex types #43644

Uh oh!

Conversation

xuhdev commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuhdev commented Aug 26, 2020

Uh oh!

mruberry commented Aug 26, 2020

Uh oh!

xuhdev commented Aug 27, 2020

Uh oh!

codecov bot commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mruberry commented Aug 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

anjali411 commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuhdev commented Sep 1, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xuhdev commented Aug 26, 2020 •

edited

Loading

codecov bot commented Aug 27, 2020 •

edited

Loading

anjali411 commented Sep 1, 2020 •

edited

Loading