Log1p for complex in CPU #89691

mfkasim1 · 2022-11-25T17:23:11Z

Another PR for #89205: making torch.log1p accepts complex numbers in CPU.
I haven't done the GPU version because I'm not sure which file(s) to change.

cc @VitalyFedyunin @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10

pytorch-bot · 2022-11-25T17:23:14Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/89691

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 Failures

As of commit 3f03070:

The following jobs have failed:

manywheel-py3_7-cuda11_6-test / build

This comment was automatically generated by Dr. CI and updates every 15 minutes.

jgong5 · 2022-11-28T12:50:01Z

test/test_unary_ufuncs.py

+            (0.2 + 0.3j, 0.21263386770217202 + 0.24497866312686414j),
+            (1e-19 + 1e-18j, 1e-19 + 1e-18j),
+            (1e-18 + 0.1j, 0.00497517 + 0.0996687j),
+            (0.1 + 1e-18j, 0.0953102 + 9.090909090909090909e-19j),
+            (0.5 + 0j, 0.40546510810816 + 0j),
+            (0.0 + 0.5j, 0.111571776 + 0.463647609j),


Consider using numpy as a ref check instead hard-coding the ref result?

numpy's log1p suffers from loss of precision with relatively small complex number (see here). That's why I'm using the hard-coded results, calculated from arbitrary-precision math and wolfram alpha.

@mfkasim1 please mention that in the code comment, otherwise someone may try to replace the hardcoded values later.

Writing a test like this is so unusual now that, as @kit1980 mentions, it absolutely needs a comment. In the future we may want to extend the test framework to automatically generate "golden value" tests.

Done adding comments on the test

We do have golden testing in some parts of PyTorch. I wonder how difficult would it be to use it here... Probably too much to be worth the effort really, but yeah.

lezcano

Left a couple comments on the testing, but otherwise it looks good.

For the complex part, you can see how it's done for cuda/UnaryLogKernels.cu. You may want to do this in a follow-up PR.

lezcano · 2022-12-01T13:48:02Z

test/test_unary_ufuncs.py

@@ -1094,6 +1094,48 @@ def test_mish(self, device, dtype):
            rtol=rtol,
        )

+    @dtypes(torch.complex64, torch.complex128)
+    @onlyCPU
+    def test_log1p_complex(self, device, dtype):


Given that you added vectorisation paths for this fucntion, it'd be good to also test those. In particular, I think that running it on tensors of size 8 should already do the trick.

Actually, this may something that may be of interest in general for unary and binary ufuncs. See test_unary_ufuncs.py. Perhaps someone from intel may be interested in implementing this? @jgong5 The reference for the test can be either numpy or a non-contiguous tensor.

Actually, this may something that may be of interest in general for unary and binary ufuncs. See test_unary_ufuncs.py. Perhaps someone from intel may be interested in implementing this? @jgong5 The reference for the test can be either numpy or a non-contiguous tensor.

You are talking about a way of test the vector and non-vector implementation in general, right? I noticed that a lot of tests in test_unary_ufuncs.py already use a big size and should hit the vector path already (even though the scalar path might not be covered then). Some tests use smaller size and the vector path is not covered though. This would need a more thorough review to know the current status. We can probably make sure all the implementations can trigger the vector path and then control the ATEN_CPU_CAPABILITY env var to cover both vector and non-vector paths with the same test code. cc @sanchitintel and @mingfeima for their opinions.

So, what I've seen is that there are some one-offs tests for some ops, but we could potentially have a dedicated test that tests the vectorised path against the non-vectorised explicitly. The plan to test depending on the CPU capabilities sounds great as well!

lezcano · 2022-12-01T13:53:05Z

test/test_unary_ufuncs.py

+            (0.2 + 0.3j, 0.21263386770217202 + 0.24497866312686414j),
+            (1e-19 + 1e-18j, 1e-19 + 1e-18j),
+            (1e-18 + 0.1j, 0.00497517 + 0.0996687j),
+            (0.1 + 1e-18j, 0.0953102 + 9.090909090909090909e-19j),
+            (0.5 + 0j, 0.40546510810816 + 0j),
+            (0.0 + 0.5j, 0.111571776 + 0.463647609j),


We do have golden testing in some parts of PyTorch. I wonder how difficult would it be to use it here... Probably too much to be worth the effort really, but yeah.

jgong5

LGTM

mfkasim1 · 2022-12-05T09:42:42Z

What’s the next step for this?

lezcano · 2022-12-05T09:44:23Z

We should add tests for the vectorised paths. Once that's done we can merge this.

lezcano

Great, thank you!

lezcano · 2022-12-05T17:58:28Z

@pytorchbot merge

pytorchmergebot · 2022-12-05T18:00:11Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

pytorchmergebot · 2022-12-05T18:10:17Z

Merge failed

Reason: The following mandatory check(s) failed (Rule Core Reviewers):

pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

lezcano · 2022-12-06T19:10:22Z

@pytorchbot merge -f "unrelated error"

pytorchmergebot · 2022-12-06T19:12:19Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

mfkasim1 · 2022-12-07T10:43:05Z

Thanks @lezcano, @jgong5, @kit1980, @mruberry for your help!

Another PR for pytorch#89205: making torch.log1p accepts complex numbers in CPU. I haven't done the GPU version because I'm not sure which file(s) to change. Pull Request resolved: pytorch#89691 Approved by: https://github.com/jgong5, https://github.com/lezcano

mfkasim1 requested review from mruberry and ngimel as code owners November 25, 2022 17:23

github-actions bot added the module: cpu CPU specific problem (e.g., perf, algorithm) label Nov 25, 2022

pytorchbot added the open source label Nov 25, 2022

mfkasim1 added 2 commits November 25, 2022 17:54

log1p complex for CPU only

6542286

Added unary test for log1p complex

5bf5276

mfkasim1 force-pushed the log1p4 branch from 594ae52 to 5bf5276 Compare November 25, 2022 17:55

mfkasim1 added 2 commits November 26, 2022 06:38

Remove log1p from cmath jit test (it's unavailable)

de6fbe2

Added complex type to log1p opinfo

7213713

jgong5 reviewed Nov 28, 2022

View reviewed changes

albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 29, 2022

mfkasim1 added 2 commits November 29, 2022 15:27

Added comments on why not using np.log1p

613687a

lintrunner

270b57b

mfkasim1 requested review from jgong5 and kit1980 and removed request for ngimel, mruberry, jgong5 and kit1980 November 30, 2022 13:30

lezcano reviewed Dec 1, 2022

View reviewed changes

jgong5 approved these changes Dec 1, 2022

View reviewed changes

Added test log1p with tensor inputs

3f03070

lezcano approved these changes Dec 5, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Dec 5, 2022

pytorchmergebot added the Merged label Dec 6, 2022

pytorchmergebot closed this in ce21262 Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log1p for complex in CPU #89691

Log1p for complex in CPU #89691

mfkasim1 commented Nov 25, 2022 •

edited by pytorch-bot bot

pytorch-bot bot commented Nov 25, 2022 •

edited

jgong5 Nov 28, 2022

mfkasim1 Nov 28, 2022

kit1980 Nov 29, 2022

mruberry Nov 29, 2022

mfkasim1 Nov 29, 2022

lezcano Dec 1, 2022

lezcano left a comment

lezcano Dec 1, 2022

lezcano Dec 1, 2022

jgong5 Dec 1, 2022

lezcano Dec 1, 2022

lezcano Dec 1, 2022

jgong5 left a comment

mfkasim1 commented Dec 5, 2022

lezcano commented Dec 5, 2022

lezcano left a comment

lezcano commented Dec 5, 2022

pytorchmergebot commented Dec 5, 2022

pytorchmergebot commented Dec 5, 2022

lezcano commented Dec 6, 2022

pytorchmergebot commented Dec 6, 2022

mfkasim1 commented Dec 7, 2022

Log1p for complex in CPU #89691

Log1p for complex in CPU #89691

Conversation

mfkasim1 commented Nov 25, 2022 • edited by pytorch-bot bot

pytorch-bot bot commented Nov 25, 2022 • edited

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/89691

❌ 1 Failures

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lezcano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgong5 left a comment

Choose a reason for hiding this comment

mfkasim1 commented Dec 5, 2022

lezcano commented Dec 5, 2022

lezcano left a comment

Choose a reason for hiding this comment

lezcano commented Dec 5, 2022

pytorchmergebot commented Dec 5, 2022

Merge started

pytorchmergebot commented Dec 5, 2022

Merge failed

lezcano commented Dec 6, 2022

pytorchmergebot commented Dec 6, 2022

Merge started

mfkasim1 commented Dec 7, 2022

mfkasim1 commented Nov 25, 2022 •

edited by pytorch-bot bot

pytorch-bot bot commented Nov 25, 2022 •

edited