Skip to content
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

improved definitions of ishermitian/issymmetric #1266

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

lxvm
Copy link
Contributor

@lxvm lxvm commented Jun 25, 2024

As mentioned in #1263, the SHermitianCompact type can represent matrices which are neither Hermitian nor symmetric, so this pr fixes the definitions of ishermitian and issymmetric to capture this and adds tests to verify the logic on some of these unusual cases. I think this would be preferable to making SHermitianCompact guarantee that it is in fact Hermitian, i.e. by always returning real values on the diagonal, because the latter would be breaking.

@mateuszbaran
Copy link
Collaborator

LGTM. I wouldn't necessarily add a guarantee that SHermitianCompact is hermitian but I think we should fix rand for this type so that it only returns hermitian matrices.

@lxvm
Copy link
Contributor Author

lxvm commented Jun 26, 2024

Thanks, now that the correctness issue of whether the matrix is in fact Hermitian or symmetric is addressed, I don't think we want people to get the impression that this type is in fact mathematically Hermitian by returning random Hermitian matrices. I will add some documentation that explains the caveat that the diagonal may not be real and a formula for making a SHermitianCompact matrix Hermitian, or simply using the Hermitian wrapper.

@mateuszbaran
Copy link
Collaborator

OK, let's keep it as-is then.

@mateuszbaran mateuszbaran merged commit 31c09e9 into JuliaArrays:master Jun 27, 2024
19 of 24 checks passed
@lxvm lxvm deleted the ishermitian branch July 2, 2024 11:27
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.

2 participants