Skip to content

BUG: np.linalg.svd(..., hermitian=True) returns non-unitary vh#31347

Open
EarlMilktea wants to merge 2 commits into
numpy:mainfrom
EarlMilktea:svdh-singular
Open

BUG: np.linalg.svd(..., hermitian=True) returns non-unitary vh#31347
EarlMilktea wants to merge 2 commits into
numpy:mainfrom
EarlMilktea:svdh-singular

Conversation

@EarlMilktea
Copy link
Copy Markdown
Contributor

@EarlMilktea EarlMilktea commented Apr 28, 2026

PR summary

Currently np.linalg.svd can return non-unitary vh due to sign 0 appearing when input array is singular.
This PR fixes the issue.

>>> import numpy as np
>>> res = np.linalg.svd(np.array([[1, 0], [0, 0]]), hermitian=True)
>>> res.Vh
array([[1., 0.],
       [0., 0.]])

AI Disclosure

I've used ChatGPT to ensure that my PR is following the guideline. No codes are generated by AI.

Explicitly remove 0 from eigenvalue signs to ensure that vh is unitary
sign contains zero here as x @ x = [[1, 0], [0, 0]].
@EarlMilktea EarlMilktea marked this pull request as ready for review April 28, 2026 05:36
@EarlMilktea
Copy link
Copy Markdown
Contributor Author

MEMO: I suspect two CI failures are not related to this PR.

Comment thread numpy/linalg/_linalg.py
s, u = eigh(a)
sgn = sign(s)
# avoid zero sign
sgn = np.where(sgn == 0, 1, sgn)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, can we assume that -0 isn't a thing and just use signbit instead of sign?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I personally prefer my approach to make the result predictable...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

well, there's a performance penalty with this approach

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, but if we aren't sure (I dunno yet), there is already sorting, etc. going on, so it shouldn't actually matter (where just always feels slightly awkward to me, I guess).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Anyway, I think it's probably good. But @WarrenWeckesser might have a quick thought and know this a bit better.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you all for comments. Let me know if there are any updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants