Make `torch.det()` support complex input. #45980

nikitaved · 2020-10-07T17:45:45Z

As per title. A minor fix required to make it available for the CPU (fmod does not support complex).
For CUDA requires #45898 .

nikitaved · 2020-10-07T17:49:32Z

Once #45898 is merged, I plan to extend the existing tests to complex types.

codecov · 2020-10-07T21:54:34Z

Codecov Report

Merging #45980 into master will increase coverage by 24.78%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master   #45980       +/-   ##
===========================================
+ Coverage   36.02%   60.81%   +24.78%     
===========================================
  Files         437     2749     +2312     
  Lines       55230   254094   +198864     
===========================================
+ Hits        19898   154530   +134632     
- Misses      35332    99564    +64232

torch/testing/_internal/common_methods_invocations.py

anjali411 · 2020-10-08T18:19:38Z

test/test_torch.py


+    # no CUDA LU yet, used for torch.det())
+    @onlyCPU
+    @skipCPUIfNoLapack


@mruberry should this test go in test_unary_ops or test_linalg?

test_linalg

alright then @nikitaved we should just update this test:

pytorch/test/test_linalg.py

Line 162 in 8e8fb85

def test_det(self, device, dtype):

ping @nikitaved please merge this test with the test in test_linalg.py

Hmm, when I try moving things into test_linalg.py, some checks fail, unlike for test_torch.py.
For example, the test for upper/lower-diagonal matrices fail, as the product of diagonal elements does not match the det result. No problem in test_torch.py.

All fine, the tests are for quite large tensors, so we run into overflow issues.

anjali411

@nikitaved this looks good overall. could you update the autograd test for det to run for complex? also let's just extend the test for det in test_linalg.py

aten/src/ATen/native/LinearAlgebra.cpp

ngimel · 2020-10-15T01:33:54Z

aten/src/ATen/native/LinearAlgebra.cpp

+  auto num_exchanges = (at::arange(1, n + 1, pivs.options().dtype(at::kLong)) != pivs.to(at::kLong))
+    .sum(-1, /*keepdim=*/false, /*dtype=*/at::kLong).fmod_(2);
  // NB: the `.contiguous()` call is added due to the bug in `.prod()` as reported in
  // issue #https://github.com/pytorch/pytorch/issues/34061


#34061 looks to be fixed, should we remove this workaround?

to @ngimel 's point, let's remove this workaround since #34061 is fixed

test/test_torch.py

nikitaved · 2020-10-19T14:41:31Z

Thank you very much for your input! I updated the PR. The tests from common_methods_invocations.py are disabled as backward for SVD is not yet implemented for complex.

@anjali411 , could you please tell whether there is still a limitation of autograd not being able to process functions with several outputs of complex type?

dr-ci · 2020-10-19T14:49:50Z

💊 CI failures summary and remediations

As of commit ae6ebd4 (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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 21 times.

nikitaved · 2020-10-19T15:04:08Z

aten/src/ATen/native/cuda/ReduceSumProdKernel.cu

+    AT_DISPATCH_ALL_TYPES_AND_COMPLEX(iter.dtype(), "prod_cuda", [&]() {
      prod_functor<scalar_t>{}(iter);
    });


Add tests for complex prod. Does it make sense to do it in a separate PR? The backward has to get modified accordingly.

NOTE: backward for prod will not work for complex inputs, because it relies on nonzero, which is not yet implemented for the complex numbers.

yes this sounds good let's update the backward formula in the next PR. This PR #46596 enables backward for nonzero for complex. In general, to enable backward for an op for complex, you would need to add an entry in GRADIENT_IMPLEMENTED_FOR_COMPLEX in gen_variable.py.

anjali411 · 2020-10-23T18:27:45Z

could you please tell whether there is still a limitation of autograd not being able to process functions with several outputs of complex type?

No I don't think so. Please let me know if you are running into any issues though

anjali411

lgtm

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-10-30T17:31:32Z

Hi @nikitaved!

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but we do not have a signature on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

anjali411 · 2020-11-02T16:10:07Z

@nikitaved can you sign the CLA, rebase the PR and remove the .contiguous() workaround please?

nikitaved · 2020-11-03T20:05:18Z

@anjali411 , sorry, force-pushed from the wrong machine, now it should be fixed.

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-11-05T03:18:56Z

@anjali411 merged this pull request in 8a3728c.

anjali411 · 2020-11-05T15:47:18Z

@nikitaved would you like to create a follow-up PR to add complex autograd support for torch.det and torch.prod?

nikitaved · 2020-11-17T20:30:42Z

@anjali411 , turns out torch.det is a rabbit hole, its backward testing does depend on prod, cumsum, cumprod, index_select, index_add and counting (gradgradcheck I presume). Since all these methods have quite an extensive test coverage, I will submit one PR per each.

mruberry · 2020-11-18T10:51:19Z

Hey @nikitaved! @anjali411 is actually on pto at the moment, but that plan makes a lot of sense.

I added complex autograd support for torch.linalg.det as a task in the linear algebra tacking issue (#42666) with a link to your comment. Let's track the work there.

nikitaved · 2020-11-18T12:14:47Z

@mruberry , thank you, will update the issue there. Meanwhile, can I request your reviews on these PRs?

nikitaved · 2020-11-18T13:45:34Z

It turns out there is a circular relationship between dependencies, so, it is going to be a single PR.

mruberry · 2020-11-18T17:54:28Z

@mruberry , thank you, will update the issue there. Meanwhile, can I request your reviews on these PRs?

Of course.

nikitaved added the module: complex Related to complex number support in PyTorch label Oct 7, 2020

nikitaved requested a review from anjali411 October 7, 2020 17:46

pytorchbot added the open source label Oct 7, 2020

ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 7, 2020

anjali411 requested changes Oct 8, 2020

View reviewed changes

anjali411 requested changes Oct 14, 2020

View reviewed changes

ngimel reviewed Oct 15, 2020

View reviewed changes

nikitaved commented Oct 19, 2020

View reviewed changes

IvanYashchuk mentioned this pull request Oct 23, 2020

Complex Numbers Support #33152

Closed

anjali411 approved these changes Oct 23, 2020

View reviewed changes

facebook-github-bot reviewed Oct 26, 2020

View reviewed changes

facebook-github-bot added the cla signed label Nov 2, 2020

nikitaved force-pushed the nikved/complex_det branch from b5d9529 to 625191f Compare November 3, 2020 11:00

complex det

ae6ebd4

nikitaved force-pushed the nikved/complex_det branch from 625191f to ae6ebd4 Compare November 3, 2020 20:04

facebook-github-bot reviewed Nov 4, 2020

View reviewed changes

facebook-github-bot closed this in 8a3728c Nov 5, 2020

facebook-github-bot added the Merged label Nov 5, 2020

mruberry mentioned this pull request Nov 18, 2020

torch.linalg in PyTorch 1.10 tracker #42666

Closed

4 tasks

ngimel mentioned this pull request Dec 26, 2020

Determinants of complex matrices give error #49857

Closed

facebook-github-bot deleted the nikved/complex_det branch January 27, 2021 18:27

Make torch.det() support complex input. #45980

Make torch.det() support complex input. #45980

Uh oh!

Conversation

nikitaved commented Oct 7, 2020

Uh oh!

nikitaved commented Oct 7, 2020

Uh oh!

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

Uh oh!

Codecov Report

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!

nikitaved Oct 21, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

anjali411 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikitaved commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

nikitaved Oct 19, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjali411 commented Oct 23, 2020

Uh oh!

anjali411 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!

facebook-github-bot commented Oct 30, 2020

Uh oh!

anjali411 commented Nov 2, 2020

Uh oh!

nikitaved commented Nov 3, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 5, 2020

Uh oh!

anjali411 commented Nov 5, 2020

Uh oh!

nikitaved commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mruberry commented Nov 18, 2020

Uh oh!

nikitaved commented Nov 18, 2020

Make `torch.det()` support complex input. #45980

Make `torch.det()` support complex input. #45980

codecov bot commented Oct 7, 2020 •

edited

Loading

nikitaved Oct 21, 2020 •

edited

Loading

nikitaved commented Oct 19, 2020 •

edited

Loading

dr-ci bot commented Oct 19, 2020 •

edited

Loading

nikitaved Oct 19, 2020 •

edited

Loading

nikitaved commented Nov 17, 2020 •

edited

Loading

nikitaved commented Nov 18, 2020 •

edited

Loading