-
-
Couldn't load subscription status.
- Fork 35
Add hermitianpart method for Number types #1445
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
base: master
Are you sure you want to change the base?
Add hermitianpart method for Number types #1445
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1445 +/- ##
=======================================
Coverage 93.89% 93.89%
=======================================
Files 34 34
Lines 15920 15921 +1
=======================================
+ Hits 14948 14949 +1
Misses 972 972 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR!
|
I think we should have |
|
I see the point, but I’d lean towards keeping hermitianpart(x::Number) = real(x) for scalars. What do you think @stevengj ? Could this make sense? |
|
I think the general principle here is that we shouldn't change the type unless it is required to represent the output, since type conversions may incur a loss of data. |
|
I think it would be the perfect solution for the sister PR #1439, which currently has an unsatisfactory behavior: julia> Z = complex([1 0;0 2]);
julia> inv(Diagonal(Z))
2×2 Diagonal{ComplexF64, Vector{ComplexF64}}:
1.0-0.0im ⋅
⋅ 0.5-0.0im
julia> inv(Hermitian(Diagonal(Z)))
2×2 Hermitian{Float64, Diagonal{Float64, Vector{Float64}}}:
1.0 ⋅
⋅ 0.5 |
It doesn't change the underlying numeric type — it just extracts a component, like
Why? You want the meaning between both cases to be consistent, so that you can use it in generic code. But the return type is different anyway, so what does it matter if the matrix case (by necessity) also does a floating-point conversion? |
|
Because we'd get type instability. |
It's not a type instability to return different output types for different input types. |
|
It's bound to happen with any non-trivial code. It's very common to have special cases for diagonal, symmetric, Hermitian matrices in LinearAlgebra, where we wrap the matrix, do the computation, and unwrap it again. I fixed a lot of type instabilities resulting from this: #1360 Letting the inverse of a complex matrix be a real matrix is just asking for trouble. |
What is bound to happen?
What specific problem are you worried about?
I agree that it's asking for trouble, because the inverse of a complex matrix is generally not real. I don't understand what that has to do with the current PR. |
|
I think it makes sense to have hermitian(A::Number, ::Symbol=:U) = convert(typeof(A), real(A)) |
2839e35 to
8bf4fdc
Compare
|
I just saw that the tests on this PR aren't working. Do you think this could be due to code changes @dkarrasch ? Also, I'm still not sure if you prefer to keep "real." Taking a closer look, it seems like having "real" makes the most sense. Looking forward to hearing from you, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few comments. Once that is settled, we should be able to merge.
| --- | ||
| ## 🚀 About this fork | ||
|
|
||
| This is my fork of LinearAlgebra.jl for contributing to Julia as part of my GSoC 2026 preparation. | ||
|
|
||
| **My contributions:** | ||
| - [PR #1445](https://github.com/JuliaLang/LinearAlgebra.jl/pull/1445) - Add hermitianpart method for Number types | ||
| - Active in issues: Coming | ||
|
|
||
| **Original repository:** [JuliaLang/LinearAlgebra.jl](https://github.com/JuliaLang/LinearAlgebra.jl) | ||
|
|
||
| --- | ||
|
|
||
| # LinearAlgebra | ||
|
|
||
| This package is part of the Julia standard library (stdlib). | ||
|
|
||
| LinearAlgebra.jl provides functionality for performing linear algebra operations in Julia. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please remove all these changes?
| For scalar inputs `x`, the function returns the real part of `x`. Standard integer scalars | ||
| are promoted to `Float64` to align with the behavior of 1×1 Hermitian matrices, while other numeric types | ||
| (`Float32`, `BigInt`, `Rational`, `BigFloat`) are preserved. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to leave this out, too.
| !!! compat "Julia 1.10" | ||
| This function requires Julia 1.10 or later. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring needs to be attached to the function definition, otherwise Julia doesn't know where it belongs.
| """ | ||
|
|
||
| hermitianpart(A::AbstractMatrix, uplo::Symbol=:U) = Hermitian(_hermitianpart(A), uplo) | ||
| hermitianpart(x::Number) = float(real(x)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| hermitianpart(x::Number) = float(real(x)) | |
| hermitianpart(x::Number) = real(x) |
I believe this will get promoted if necessary by other mechanisms, so there is no need to enforce it per se.
| inds = A.uplo == 'U' ? ijminmax : reverse(ijminmax) | ||
| Base.replace_in_print_matrix(parent(A), inds..., s) | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| end | |
| end | |
It's common style to have an empty line at the end. No need to modify this.
| @test typeof(hermitianpart(5)) == Float64 | ||
| @test hermitianpart(2.5) == 2.5 | ||
| @test hermitianpart(-1 + 0im) == -1 | ||
| @test typeof(hermitianpart(3 + 4im)) == 3.0 | ||
| @test typeof(hermitianpart(3 + 4im)) == Float64 | ||
| @test typeof(hermitianpart(2.5 + 3im)) == Float64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @test typeof(hermitianpart(5)) == Float64 | |
| @test hermitianpart(2.5) == 2.5 | |
| @test hermitianpart(-1 + 0im) == -1 | |
| @test typeof(hermitianpart(3 + 4im)) == 3.0 | |
| @test typeof(hermitianpart(3 + 4im)) == Float64 | |
| @test typeof(hermitianpart(2.5 + 3im)) == Float64 | |
| @test hermitianpart(2.5) == 2.5 | |
| @test hermitianpart(-1 + 0im) == -1 | |
| @test typeof(hermitianpart(3 + 4im)) == 3.0 |
These type tests wouldn't be needed anymore.
Fixes #1438
This PR implements
hermitianpartforNumbertypes as requested in the issue.Changes:
hermitianpart(x::Number) = real(x)method insrc/symmetric.jltest/symmetric.jlThis resolves the
MethodErrorwhen callinghermitianparton complex numbers and enables the generic code usage mentioned in the issue.