[TS] Add complex support for more ops #54541

anjali411 · 2021-03-23T20:40:22Z

Stack from ghstack:

[TS] Add complex support for more ops #54541 [TS] Add complex support for more ops

Differential Revision: D27599114

[ghstack-poisoned]

facebook-github-bot · 2021-03-23T20:40:30Z

💊 CI failures summary and remediations

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

💚 💚 Looks good so far! There are no failures yet. 💚 💚

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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[ghstack-poisoned]

TODO: add tests [ghstack-poisoned]

ghstack-source-id: 9e614ee Pull Request resolved: #54541

TODO: add tests [ghstack-poisoned]

ghstack-source-id: b37978c Pull Request resolved: #54541

SplitInfinity · 2021-03-24T21:25:35Z

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

-    DEFINE_SCALAR_SCALAR_BINARY_OP(
+    DEFINE_INT_COMPLEX_OP(aten::log, std::log(a) / std::log(b), complex),
+    DEFINE_FLOAT_COMPLEX_OP(aten::log, std::log(a) / std::log(b), complex),
+    DEFINE_SCALAR_BINARY_OP_WITH_COMPLEX(


Suggested change

DEFINE_SCALAR_BINARY_OP_WITH_COMPLEX(

DEFINE_SCALAR_SCALAR_BINARY_OP_WITH_COMPLEX(

As discussed this should probably be DEFINE_SCALAR_SCALAR_BINARY_OP_WITH_COMPLEX to avoid naming conflicts with aten::log for tensors.

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

torch/csrc/jit/frontend/sugared_value.cpp

torch/csrc/jit/runtime/register_prim_ops.cpp

TODO: add tests [ghstack-poisoned]

ghstack-source-id: e7f5531 Pull Request resolved: #54541

TODO: add tests [ghstack-poisoned]

ghstack-source-id: 9d347f7 Pull Request resolved: #54541

SplitInfinity

Just some nits and one or two questions.

torch/csrc/jit/runtime/register_prim_ops.cpp

test/jit/test_complex.py

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Differential Revision: [D27599114](https://our.internmc.facebook.com/intern/diff/D27599114) [ghstack-poisoned]

ghstack-source-id: c82eeea Pull Request resolved: #54541

Differential Revision: [D27599114](https://our.internmc.facebook.com/intern/diff/D27599114) [ghstack-poisoned]

ghstack-source-id: b5a6d28 Pull Request resolved: #54541

test/jit/test_complex.py

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp

Differential Revision: [D27599114](https://our.internmc.facebook.com/intern/diff/D27599114) [ghstack-poisoned]

ghstack-source-id: 8661adf Pull Request resolved: #54541

Differential Revision: [D27599114](https://our.internmc.facebook.com/intern/diff/D27599114) [ghstack-poisoned]

ghstack-source-id: 16ba113 Pull Request resolved: #54541

SplitInfinity · 2021-05-12T21:30:48Z

test/jit/test_complex.py

-        def fn5(a: complex, b: int):
-            return a == b
-        def fn6(a: complex, b: int):
-            return a != b
-
-        with self.assertRaises(RuntimeError):
-            self.checkScript(fn5, (x1, 1))
-        with self.assertRaises(RuntimeError):
-            self.checkScript(fn6, (x1, 1))
-


Why have these tests been removed? I think I commented about them earlier and that's why you added them?

right I saw an AddressSanitizer failure on just ASAN after I added them. Just wanted to verify if that was it.

SplitInfinity · 2021-05-12T21:34:02Z

test/jit/test_complex.py

@@ -99,7 +106,7 @@ def checkCmath(func_name, funcs_template=funcs_template):
                    self.assertEqual(res_python, res_script, msg=msg)

        unary_ops = ['log', 'log10', 'sqrt', 'exp', 'sin', 'cos', 'asin', 'acos', 'atan', 'sinh', 'cosh',
-                     'tanh', 'asinh', 'acosh', 'atanh', 'phase']
+                     'tanh', 'asinh', 'acosh', 'atanh', 'phase', 'isinf', 'isnan', 'isfinite']

        # --- Unary ops ---
        for op in unary_ops:


One cool thing I learned about recently: https://docs.python.org/3/library/unittest.html#distinguishing-test-iterations-using-subtests

Using with self.subTest(op): here might make it clear which specific test being generated within this one test is failing.

wow looks super useful!

SplitInfinity · 2021-05-12T21:34:15Z

test/jit/test_complex.py

+        def pow_float_complex(x: float, y: complex):
+            return pow(x, y)
+
+


Suggested change

SplitInfinity · 2021-05-12T21:35:40Z

test/jit/test_complex.py

@@ -120,7 +153,6 @@ def func():
        for x in (float_consts + complex_consts):
            checkCmath(x, funcs_template=func_constants_template)

-


SplitInfinity

I noticed you got rid of tests that we added specifically because we were missing coverage for invalid comparisons between int and complex (or something like that). Can you comment on that?

SplitInfinity

Ok approving but I strongly strongly recommend trying to add those tests in a follow up diff. They're not super valuable by themselves but the error logs from the failure make me think that they might have triggered a real issue that we should look into and fix.

anjali411 · 2021-05-13T01:25:39Z

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

facebook-github-bot · 2021-05-13T06:34:37Z

@anjali411 merged this pull request in 82d7149.

Summary: Pull Request resolved: pytorch#54541 Test Plan: Imported from OSS Reviewed By: SplitInfinity Differential Revision: D27599114 Pulled By: anjali411 fbshipit-source-id: 182d4480fd788599c408bfaf0d23baf3d9a4e967

[TS] Add complex support for more ops

bb1782b

[ghstack-poisoned]

facebook-github-bot added oncall: jit Add this issue/PR to JIT oncall triage queue cla signed labels Mar 23, 2021

This was referenced Mar 23, 2021

Teach Python TS frontend to parse complex literals #52881

Closed

Add JIT support for cmath unary ops #54089

Closed

Allow creating SugaredValue for a complex valued IValue and deserialization logic for "infj" and "nanj" global constants #54328

Closed

anjali411 mentioned this pull request Mar 23, 2021

Add support for real and imag tensor attributes #54542

Closed

anjali411 added 2 commits March 23, 2021 17:12

Update on "[TS] Add complex support for more ops"

9cb8367

[ghstack-poisoned]

Update on "[TS] Add complex support for more ops"

6397a1d

TODO: add tests [ghstack-poisoned]

anjali411 added a commit that referenced this pull request Mar 24, 2021

[TS] Add complex support for more ops

ac7f15f

ghstack-source-id: 9e614ee Pull Request resolved: #54541

Update on "[TS] Add complex support for more ops"

3357c2e

TODO: add tests [ghstack-poisoned]

anjali411 mentioned this pull request Mar 24, 2021

Support cmath.rect in TS #54608

Closed

Update on "[TS] Add complex support for more ops"

7e5998c

TODO: add tests [ghstack-poisoned]

anjali411 mentioned this pull request Mar 24, 2021

Add support for real and imag tensor attributes #54611

Closed

anjali411 added 2 commits March 24, 2021 12:01

Update on "[TS] Add complex support for more ops"

899023f

TODO: add tests [ghstack-poisoned]

Update on "[TS] Add complex support for more ops"

3ee4f38

TODO: add tests [ghstack-poisoned]

anjali411 added a commit that referenced this pull request Mar 24, 2021

[TS] Add complex support for more ops

9d146bc

ghstack-source-id: b37978c Pull Request resolved: #54541

SplitInfinity reviewed Mar 24, 2021

View reviewed changes

Update on "[TS] Add complex support for more ops"

8a04e13

TODO: add tests [ghstack-poisoned]

anjali411 mentioned this pull request Mar 25, 2021

Add support for real and imag tensor attributes #54692

Closed

anjali411 added 5 commits March 25, 2021 18:41

Update on "[TS] Add complex support for more ops"

8c17ed4

TODO: add tests [ghstack-poisoned]

Update on "[TS] Add complex support for more ops"

11b9ef8

TODO: add tests [ghstack-poisoned]

Update on "[TS] Add complex support for more ops"

81b7438

TODO: add tests [ghstack-poisoned]

Update on "[TS] Add complex support for more ops"

a09110d

TODO: add tests [ghstack-poisoned]

Update on "[TS] Add complex support for more ops"

8d7985b

TODO: add tests [ghstack-poisoned]

Update on "[TS] Add complex support for more ops"

331139a

TODO: add tests [ghstack-poisoned]

anjali411 added a commit that referenced this pull request Mar 30, 2021

[TS] Add complex support for more ops

1c35673

ghstack-source-id: e7f5531 Pull Request resolved: #54541

Update on "[TS] Add complex support for more ops"

766d90e

TODO: add tests [ghstack-poisoned]

anjali411 added a commit that referenced this pull request Apr 6, 2021

[TS] Add complex support for more ops

2e800e1

ghstack-source-id: 9d347f7 Pull Request resolved: #54541

SplitInfinity requested review from SplitInfinity and removed request for SplitInfinity April 8, 2021 03:34

SplitInfinity reviewed Apr 13, 2021

View reviewed changes

Update on "[TS] Add complex support for more ops"

cd11457

Differential Revision: [D27599114](https://our.internmc.facebook.com/intern/diff/D27599114) [ghstack-poisoned]

anjali411 added a commit that referenced this pull request Apr 25, 2021

[TS] Add complex support for more ops

4e4568b

ghstack-source-id: c82eeea Pull Request resolved: #54541

anjali411 requested a review from SplitInfinity May 6, 2021 18:52

Update on "[TS] Add complex support for more ops"

8850347

Differential Revision: [D27599114](https://our.internmc.facebook.com/intern/diff/D27599114) [ghstack-poisoned]

Update on "[TS] Add complex support for more ops"

d81c71d

Differential Revision: [D27599114](https://our.internmc.facebook.com/intern/diff/D27599114) [ghstack-poisoned]

anjali411 added a commit that referenced this pull request May 6, 2021

[TS] Add complex support for more ops

2faa7ce

ghstack-source-id: b5a6d28 Pull Request resolved: #54541

SplitInfinity reviewed May 6, 2021

View reviewed changes

test/jit/test_complex.py Outdated Show resolved Hide resolved

torch/csrc/jit/runtime/register_prim_ops_fulljit.cpp Show resolved Hide resolved

Update on "[TS] Add complex support for more ops"

8eb5f9d

Differential Revision: [D27599114](https://our.internmc.facebook.com/intern/diff/D27599114) [ghstack-poisoned]

anjali411 added a commit that referenced this pull request May 12, 2021

[TS] Add complex support for more ops

c0293c0

ghstack-source-id: 8661adf Pull Request resolved: #54541

anjali411 requested a review from SplitInfinity May 12, 2021 13:29

Update on "[TS] Add complex support for more ops"

3516612

Differential Revision: [D27599114](https://our.internmc.facebook.com/intern/diff/D27599114) [ghstack-poisoned]

anjali411 added a commit that referenced this pull request May 12, 2021

[TS] Add complex support for more ops

c68d2cb

ghstack-source-id: 16ba113 Pull Request resolved: #54541

anjali411 added the module: complex Related to complex number support in PyTorch label May 12, 2021

SplitInfinity reviewed May 12, 2021

View reviewed changes

test/jit/test_complex.py

def pow_float_complex(x: float, y: complex):

return pow(x, y)

Copy link

SplitInfinity May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

SplitInfinity reviewed May 12, 2021

View reviewed changes

SplitInfinity approved these changes May 13, 2021

View reviewed changes

facebook-github-bot closed this in 82d7149 May 13, 2021

facebook-github-bot added the Merged label May 13, 2021

	DEFINE_SCALAR_BINARY_OP_WITH_COMPLEX(
	DEFINE_SCALAR_SCALAR_BINARY_OP_WITH_COMPLEX(

		def pow_float_complex(x: float, y: complex):
		return pow(x, y)

		@@ -120,7 +153,6 @@ def func():
		for x in (float_consts + complex_consts):
		checkCmath(x, funcs_template=func_constants_template)

[TS] Add complex support for more ops #54541

[TS] Add complex support for more ops #54541

Uh oh!

Conversation

anjali411 commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

SplitInfinity Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SplitInfinity left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SplitInfinity May 12, 2021

Choose a reason for hiding this comment

Uh oh!

anjali411 May 12, 2021

Choose a reason for hiding this comment

Uh oh!

SplitInfinity May 12, 2021

Choose a reason for hiding this comment

Uh oh!

anjali411 May 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SplitInfinity May 12, 2021

Choose a reason for hiding this comment

Uh oh!

SplitInfinity May 12, 2021

Choose a reason for hiding this comment

Uh oh!

SplitInfinity left a comment

Choose a reason for hiding this comment

Uh oh!

SplitInfinity left a comment

Choose a reason for hiding this comment

Uh oh!

anjali411 commented May 13, 2021

Uh oh!

facebook-github-bot commented May 13, 2021

Uh oh!

Uh oh!

anjali411 commented Mar 23, 2021 •

edited

Loading

facebook-github-bot commented Mar 23, 2021 •

edited

Loading

anjali411 May 12, 2021 •

edited

Loading