torch.isreal #41298

Justin-Huber · 2020-07-11T02:21:37Z

@mruberry
Not entirely sure if all the changes are necessary in how functions are added to Pytorch.

Should it throw an error when called with a non-complex tensor? Numpy allows non-complex arrays in its imag() function which is used in its isreal() function but Pytorch's imag() throws an error for non-complex arrays.

Where does assertONNX() get its expected output to compare to?

dr-ci · 2020-07-11T02:51:07Z

💊 CI failures summary and remediations

As of commit ebe1aa7 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

1/1 tentatively recognized as flaky ❄️
- Click here to rerun these jobs

❄️ 1 failure tentatively classified as flaky

but reruns have not yet been triggered to confirm:

pytorch_linux_xenial_cuda10_2_cudnn7_py3_gcc7_test (1/1)

Step: "Download Docker image" (full log | diagnosis details | 🔁 rerun) ❄️

9VDo2n92R6UHZ5iqk9Wijq6QqwZ6vxJZZxNAR6QcR8am8RNqxOYjGmt4ZTx9t1P4n4AueNO4Gbl67a3UVVA9baalepMQdsiEJ2xGbXES4%2BlSM%2FKt8odC3JXR6uky4MtTDUpyfd5gEFqbjcItblHQzFcchLBrOXJars0MMprQPeAbspPjAV7ZlHARBtx4EQ%3D%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20200717T234123Z&X-Amz-SignedHeaders=host&X-Amz-Expires=3600&X-Amz-Credential=ASIAYTIFIPBEANZ7EANH%2F20200717%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Signature=dd144fad668c078bc7a087416b119b840236a149d9f72dfb17fc6f55719ba999: net/http: TLS handshake timeout

DOCKER_IMAGE: 308535385114.dkr.ecr.us-east-1.amazonaws.com/pytorch/pytorch-linux-xenial-cuda10.2-cudnn7-py3-gcc7:ab1632df-fa59-40e6-8c23-98e004f61148-ebe1aa73751a73b302cd1fd86e0ba006696b2d4e 
Parallel backend flags:  
VDo2n92R6UHZ5iqk9Wijq6QqwZ6vxJZZxNAR6QcR8am8RNqxOYjGmt4ZTx9t1P4n4AueNO4Gbl67a3UVVA9baalepMQdsiEJ2xGbXES4%2BlSM%2FKt8odC3JXR6uky4MtTDUpyfd5gEFqbjcItblHQzFcchLBrOXJars0MMprQPeAbspPjAV7ZlHARBtx4EQ%3D%3D&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Date=20200717T234123Z&X-Amz-SignedHeaders=host&X-Amz-Expires=3600&X-Amz-Credential=ASIAYTIFIPBEANZ7EANH%2F20200717%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Signature=dd144fad668c078bc7a087416b119b840236a149d9f72dfb17fc6f55719ba999: net/http: TLS handshake timeout

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 16 times.

kshitij12345 · 2020-07-11T12:49:49Z

@Justin-Huber
Great work!

The relevant tests should be added in test_torch.py under the TestTorchDevice class, testing with all dtypes available.

To compare it against numpy you can search for the usage of self.compare_with_numpy in test_torch.py

You can use self.assertRaises to verify the expected error is raised where the dtype is not supported.

mruberry · 2020-07-14T06:41:32Z

Be sure to take this out of draft and ping me when it's ready for review again.

Should it throw an error when called with a non-complex tensor? Numpy allows non-complex arrays in its imag() function which is used in its isreal() function but Pytorch's imag() throws an error for non-complex arrays.

It's OK to throw an error for now but I would add a note explaining why the error is thrown and ensure that we test the error happens. That way if someone fixes imag() they'll know to update the test.

Where does assertONNX() get its expected output to compare to?

I'm not totally sure what assertONNX does but I think it just verifies that the module can be translated to ONNX. If it compares output it probably compares the PyTorch output with the ONNX output.

test/onnx/test_operators.py

test/test_torch.py

mruberry · 2020-07-14T06:45:19Z

test/test_torch.py

+
+    @unittest.skipIf(not TEST_NUMPY, 'NumPy not found')
+    @dtypes(torch.float32)
+    def test_isreal_nan(self, device, dtype):


I think you can remove this test and add float('nan') to the previous test if it stops comparing with NumPy.

I separated this to declutter the noncomplex test, since we'd have to separate int from floats using NaN there.

test/test_torch.py

torch/_torch_docs.py

Justin-Huber · 2020-07-14T22:56:23Z

@mruberry It's ready to go. I changed the nan test case a bit to include nan and inf checks on the imaginary part of complex nums

mruberry · 2020-07-16T07:46:47Z

aten/src/ATen/native/native_functions.yaml

+- func: isreal(Tensor self) -> Tensor
+  use_c10_dispatcher: full
+  variants: function, method
+  device_guard: False


Why set the device guard to false? Do the at::ones_like and at::imag calls set the device guard?

This was just a copy-paste. I just checked and at::ones_like doesn't set device_guard and at::imag sets device_guard to False. What determines setting it to False?

I'm not totally sure. In some special cases it may be OK, but I would just remove the "device_guard" line for safety.

test/test_torch.py

mruberry · 2020-07-16T07:51:05Z

Hey @Justin-Huber, this looks great! Just have one question and one cleanup suggestion.

Justin-Huber · 2020-07-17T03:13:56Z

Not sure why the one build is failing @mruberry

mruberry · 2020-07-17T03:15:55Z

Not sure why the one build is failing @mruberry

Don't worry about it; it's a hardware failure unrelated to your PR.

mruberry · 2020-07-17T05:40:11Z

Not sure why the one build is failing @mruberry

Is this ready for another review, @Justin-Huber?

Justin-Huber · 2020-07-17T15:17:22Z

Yep! @mruberry

mruberry · 2020-07-17T16:27:26Z

torch/_torch_docs.py

+           r"""
+isreal(input) -> Tensor
+
+Returns a new tensor with boolean elements representing if each element is real-valued or not.


"if each element of :attr:input is ..."

mruberry · 2020-07-17T16:28:04Z

torch/_torch_docs.py

+All real-valued types are considered real. Complex values are considered real when their imaginary part is 0.
+
+Arguments:
+    input (Tensor): A tensor to check


You can just use "{input}" here and a suitable string will be autogenerated

mruberry · 2020-07-17T16:28:51Z

torch/_torch_docs.py

+    input (Tensor): A tensor to check
+
+Returns:
+    Tensor: A boolean tensor containing a True at each location of real elements.


"a boolean tensor with True where :attr:input is real-valued and False elsewhere"

mruberry

Cool! Nice work, @Justin-Huber. Just a few doc fixes which should be very easy to make and we'll get this in. Just ping me when the strings are updated.

mruberry · 2020-07-17T19:47:24Z

torch/_torch_docs.py

+           r"""
+isreal(input) -> Tensor
+
+Returns a new tensor with boolean elements representing if each element of :attr:input is real-valued or not.


This is good but you need the backticks around "input" for this (and the reference below) to format properly:

:attr:`input`

facebook-github-bot

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

Justin-Huber · 2020-07-18T01:07:17Z

@mruberry Everything good to go?

mruberry · 2020-07-18T04:14:24Z

@mruberry Everything good to go?

Yep. Just has to go through the internal landing process.

mruberry · 2020-07-18T05:11:11Z

It's landed! Congrats, @Justin-Huber! Let me know if you're interested in another project.

facebook-github-bot · 2020-07-18T06:05:03Z

@mruberry merged this pull request in c6d0fdd.

Justin-Huber · 2020-07-18T22:12:37Z

Awesome! And yeah definitely, is there anything meatier?

mruberry · 2020-07-20T08:17:56Z

Awesome! And yeah definitely, is there anything meatier?

A great next step would be a function like nextafter, which likely requires writing a TensorIterator kernel. Does that sound interesting?

Justin-Huber · 2020-07-20T23:36:58Z

Yeah definitely, could you point me to some resources on pytorch kernel's?

mruberry · 2020-07-21T07:48:57Z

Yeah definitely, could you point me to some resources on pytorch kernel's?

You can check out our wiki and this article by QuanSight for more on TensorIterator. The best place to look is probably at what a recent PR changed, though. Take a look at this recent PR implementing lcm and gcd: https://github.com/pytorch/pytorch/pull/40651/files, and let me know if you have additional questions.

Preliminary isreal() impl

7407808

Justin-Huber marked this pull request as draft July 11, 2020 02:21

pytorchbot added the open source label Jul 11, 2020

mruberry self-requested a review July 12, 2020 07:17

Justin-Huber added 2 commits July 12, 2020 12:38

added comparison with numpy in test_torch.py

83fa1c6

style fixes

9381645