-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Remove hacks for 6721 for typealias in symmetric.jl #21565
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
Conversation
base/linalg/symmetric.jl
Outdated
HermOrSym{T,S} = Union{Hermitian{T,S}, Symmetric{T,S}} | ||
RealHermSymComplexHerm{T<:Real,S} = Union{Hermitian{T,S}, Symmetric{T,S}, Hermitian{Complex{T},S}} | ||
const HermOrSym{T,S} = Union{Hermitian{T,S}, Symmetric{T,S}} | ||
const RealHermSymComplexHerm{T<:Real,S} = Union{Hermitian{T,S}, Symmetric{T,S}, Hermitian{Complex{T},S}} |
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.
these const
s aren't necessary
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 know, but it's a lot easier to quickly glance a file and figure out it's a typealias with the const there
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.
This can also be written
const RealHermSymComplexHerm{T,S} = Union{Hermitian{T,S}, Symmetric{T,S}, Hermitian{Complex{T},S}} where T<:Real
perhaps that is more clear ? as the right hand side matches something you would find in a function signature.
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.
This code seems pretty repetitive – could the repetition be factored out?
Probably but for a different PR. |
base/linalg/symmetric.jl
Outdated
eigfact!(S != T ? convert(AbstractMatrix{S}, A) : copy(A)) | ||
end | ||
|
||
eigfact!(A::RealHermSymComplexHerm, irange::UnitRange) = Eigen(LAPACK.syevr!('V', 'I', A.uplo, A.data, 0.0, 0.0, irange.start, irange.stop, -1.0)...) |
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 <:StridedMatrix
constraint seems necessary given the implementation via LAPACK? (Edit: Similarly, shouldn't the element type be constrained to BlasFloat
s?)
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.
yes!
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 @musm! :)
(Absent objections, requests for time, or someone else beating me to it, I plan to merge this pull request this evening or tomorrow morning. Best!) |
No description provided.