torch.isreal#41298
Conversation
💊 CI failures summary and remediationsAs of commit ebe1aa7 (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
❄️ 1 failure tentatively classified as flakybut reruns have not yet been triggered to confirm:
|
|
@Justin-Huber The relevant tests should be added in To compare it against You can use |
|
Be sure to take this out of draft and ping me when it's ready for review again.
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.
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. |
|
|
||
| @unittest.skipIf(not TEST_NUMPY, 'NumPy not found') | ||
| @dtypes(torch.float32) | ||
| def test_isreal_nan(self, device, dtype): |
There was a problem hiding this comment.
I think you can remove this test and add float('nan') to the previous test if it stops comparing with NumPy.
There was a problem hiding this comment.
I separated this to declutter the noncomplex test, since we'd have to separate int from floats using NaN there.
|
@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 |
| - func: isreal(Tensor self) -> Tensor | ||
| use_c10_dispatcher: full | ||
| variants: function, method | ||
| device_guard: False |
There was a problem hiding this comment.
Why set the device guard to false? Do the at::ones_like and at::imag calls set the device guard?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm not totally sure. In some special cases it may be OK, but I would just remove the "device_guard" line for safety.
|
Hey @Justin-Huber, this looks great! Just have one question and one cleanup suggestion. |
|
Not sure why the one build is failing @mruberry |
Don't worry about it; it's a hardware failure unrelated to your PR. |
Is this ready for another review, @Justin-Huber? |
|
Yep! @mruberry |
| r""" | ||
| isreal(input) -> Tensor | ||
|
|
||
| Returns a new tensor with boolean elements representing if each element is real-valued or not. |
There was a problem hiding this comment.
"if each element of :attr:input is ..."
| 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 |
There was a problem hiding this comment.
You can just use "{input}" here and a suitable string will be autogenerated
| input (Tensor): A tensor to check | ||
|
|
||
| Returns: | ||
| Tensor: A boolean tensor containing a True at each location of real elements. |
There was a problem hiding this comment.
"a boolean tensor with True where :attr:input is real-valued and False elsewhere"
mruberry
left a comment
There was a problem hiding this comment.
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.
| r""" | ||
| isreal(input) -> Tensor | ||
|
|
||
| Returns a new tensor with boolean elements representing if each element of :attr:input is real-valued or not. |
There was a problem hiding this comment.
This is good but you need the backticks around "input" for this (and the reference below) to format properly:
:attr:`input`
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@mruberry Everything good to go? |
Yep. Just has to go through the internal landing process. |
|
It's landed! Congrats, @Justin-Huber! Let me know if you're interested in another project. |
|
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? |
|
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. |
Summary: pytorch#38349 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? Pull Request resolved: pytorch#41298 Reviewed By: ngimel Differential Revision: D22610500 Pulled By: mruberry fbshipit-source-id: 817d61f8b1c3670788b81690636bd41335788439
#38349
@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?