Skip to content

Add complex number support to linalg.slogdet #567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 14, 2022
Merged

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Dec 13, 2022

This PR

  • adds complex number support to linalg.slogdet.
  • defines the sign of the determinant in agreement with Add complex number support to sign #556. In contrast to its sign function, NumPy uses the definition $\operatorname{sign}(\det x) = \frac{\det x}{|\det x|}$ to compute the sign of the determinant. At the time of this PR, PyTorch refers to the "sign" as the angle (which I take to mean the argument of the determinant); however, I don't believe this is correct, as, via polar decomposition, $\det A = |\det A| e^{\theta j}$. Given the expected equality that $\det A = \operatorname{sign}(\det A) \cdot e^{\ln |\det A|} = |\det A| \operatorname{sign}(\det A)$, we see that what should be returned is $e^{\theta j}$, not simply the argument $\theta$. While the PyTorch docs state that the angle is returned, what is actually returned is $\frac{\det x}{|\det x|}$.
  • updates the allowed data types from real-valued floating-point to any floating-point data type.
  • requires that the sign array be the same data type as the input array and logabsdet be unconditionally real-valued.
  • adds an extended description describing the operation.
  • makes explicit that, for matrix stacks, a solution must be computed for each matrix in the stack.

@kgryte kgryte added API change Changes to existing functions or objects in the API. topic: Complex Data Types Complex number data types. topic: Linear Algebra Linear algebra. labels Dec 13, 2022
@kgryte kgryte added this to the v2022 milestone Dec 13, 2022
@rgommers
Copy link
Member

... we see that what should be returned is $e^{\theta j}$, not simply the argument $\theta$. While the PyTorch docs state that the angle is returned, what is actually returned is $\frac{\det x}{|\det x|}$.

I think that that's mostly a terminology thing? The implementations are consistent:

>>> x = np.array([[0.1+0.9j, 0.5-0.5j], [1, 2j]])
>>> np.linalg.slogdet(x)
((-0.9566738804288585+0.29116161578269606j), 0.877201841342143)
>>> torch.linalg.slogdet(torch.as_tensor(x))
torch.return_types.linalg_slogdet(
sign=tensor(-0.9567+0.2912j, dtype=torch.complex128),
logabsdet=tensor(0.8772, dtype=torch.float64))

If there's something to clarify or improve in the PyTorch wording in the slogdet description, that's of course useful. @lezcano since you wrote those docs, you may want to have a look at the comment in the PR description here.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Changes here LGTM, thanks @kgryte

@rgommers rgommers merged commit a1d7edc into main Dec 14, 2022
@rgommers rgommers deleted the cmplx-linalg-slogdet branch December 14, 2022 19:46
@lezcano
Copy link
Contributor

lezcano commented Dec 15, 2022

I agree that the term angle is not correct. The quantity that's returned in PyTorch is indeed the sign, and the rest of the docs and the implementation are consistent with this. I'll fix that in the PyTorch docs soon.

lezcano added a commit to pytorch/pytorch that referenced this pull request Dec 19, 2022
This issue was raised in data-apis/array-api#567

[ghstack-poisoned]
lezcano added a commit to pytorch/pytorch that referenced this pull request Dec 19, 2022
This issue was raised in data-apis/array-api#567

ghstack-source-id: e9cd318cd8957215c5672925298ad86c97fe3283
Pull Request resolved: #91129
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Changes to existing functions or objects in the API. topic: Complex Data Types Complex number data types. topic: Linear Algebra Linear algebra.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants