Skip to content

Conversation

vandenman
Copy link

@vandenman vandenman commented Sep 11, 2025

Fixes #1437

Does not add a test for 1438, but let me know if I should.

Feedback is welcome!

Copy link

codecov bot commented Sep 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.90%. Comparing base (98723df) to head (af1fe00).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1439   +/-   ##
=======================================
  Coverage   93.89%   93.90%           
=======================================
  Files          34       34           
  Lines       15920    15922    +2     
=======================================
+ Hits        14948    14951    +3     
+ Misses        972      971    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@araujoms
Copy link
Collaborator

araujoms commented Sep 11, 2025

You do need to add a test for #1438.

You probably want to preserve the type of the number in hermitianpart, to match the behaviour when acting on 1x1 matrices. With something like _hermitianpart(a::T) where {T<:Number} = T(real(a)).

You probably want to do something similar for inv(A::Hermitian{<:Number,<:Diagonal}), it's weird for a complex matrix to suddenly become real.

@stevengj
Copy link
Member

Since #1438 is not needed for this PR, it should be a separate PR.


# Ensure doubly wrapped matrices use efficient diagonal methods and return a Symmetric/Hermitian type
inv(A::Symmetric{<:Number,<:Diagonal}) = Symmetric(inv(A.data), sym_uplo(A.uplo))
inv(A::Hermitian{<:Number,<:Diagonal}) = Hermitian(inv(real(A.data)), sym_uplo(A.uplo))
Copy link
Member

Choose a reason for hiding this comment

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

To avoid the issue of returning a real-eltype matrix for a complex-eltype argument, this should perhaps be

Suggested change
inv(A::Hermitian{<:Number,<:Diagonal}) = Hermitian(inv(real(A.data)), sym_uplo(A.uplo))
inv(A::Hermitian{T,S}) where {T<:Number, S<:Diagonal} = Hermitian{T,S}(inv(real(A.data)), A.uplo)

?

Copy link
Author

Choose a reason for hiding this comment

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

I suppose that's possible, but a Hermitian Diagonal matrix is actually always a real Diagonal matrix, so we can deduce from the type alone that the inverse is always a real diagonal matrix. This also holds for quaternion and octonion valued matrices. If there is a number type where becomes a problem, then using real might also be a problem.

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.

Inverse of Symmetric{ , Diagonal} returns a dense matrix
4 participants