-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
WIP: Improve accuracy of ^ for ill-conditioned matrices #12584
Conversation
"Improve" is a little vague. Does this change behavior or just performance? Some explanation and examples of the difference would be nice. |
@StefanKarpinski You're right, an explanation might help. The implementation that we have now in julia> e= 1e-16; A = [1+e 1; 0 1];
julia> cond(A)
2.618033988749896
julia> F = eigfact(A); cond(F.vectors)
9.007199254740992e15 On julia> norm((A^(1/2))^2 - A) / norm(A)
0.6180339887498948 and on julia> norm((A^(1/2))^2 - A) / norm(A)
6.861555643110582e-17 To answer your question, I'd say that the focus is on the behaviour here. |
warn("Matrix with nonpositive real eigenvalues, a nonprincipal matrix power will be returned.") | ||
end | ||
|
||
if isreal(A) && ~np_real_eigs |
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.
Are we worried about type-stability of this function? Or is it slow enough that we don't care?
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.
If we can be type stable without sacrificing too much convenience, it's better to, even if things aren't performance critical.
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.
So far we haven't been type stable for matrix functions like this, e.g. eig
and sqrtm
return Float64
for Float64
input if possible. If we want type stability, we could
- Mimic scalar function and return
DomainError
- Always return
Complex
I think option 1 is a no go because the consequence would be to solve real eigenvalue problems in complex arithmetic which is much more expensive. The main problem with option two is that some people might be annoyed by the complex return matrix when all entries are real valued, but that might be a reasonable price for type stability.
@stevengj Thank you for checking the patch thoroughly. I addressed most of the points you raised, still to do:
I'll make this PR a WIP and will probably add one or two other commits (tests and docs). Not sure I found the least expensive way to do the in place operations, so feel free to check the loops again (I'll mark them with a line comment for convenience). |
I managed to push the use of |
@@ -976,18 +976,110 @@ for (f, g) in ((:/, :A_rdiv_B!), (:A_rdiv_Bc, :A_rdiv_Bc!), (:A_rdiv_Bt, :A_rdiv | |||
end | |||
end | |||
|
|||
function ^(A::UpperTriangular, p::Integer) |
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 think it would probably be clearer, and consistent with standard practice in linear algebra, to name your upper-triangular matrices U
instead of A
, here and below.
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.
Agreed, but I'd rather prefer not to do it with this PR, because it would break the consistency with the notation of sqrtm
. I'll put it in my TODO list.
…mpact broadcast syntax.
…pact broadcast syntax.
…ompact broadcast syntax.
…pact broadcast syntax.
…compact broadcast syntax.
…act broadcast syntax.
…t broadcast syntax.
… broadcast syntax.
… compact broadcast syntax.
… compact broadcast syntax.
…of compact broadcast syntax.
…pact broadcast syntax.
…ons and deprecation of `@vectorize_1arg` and `@vectorize_2arg` themselves. Also add cross-referenes between max and maximum, and also between min and minimum.
…aries and enabling broadcast loop fusion.
…18342) * don't throw DomainError for negative integer powers of ±1 * fix replutil test that was expecting 1^(-1) to fail
Uh oh, looks like something went terribly wrong with a rebase... |
@stevengj Yes, you're right. No Idea how to fix this (I'm trying but it looks like I went a git push -f too far), I think I will have to start over with a new branch |
@mfasi To avoid making a new PR you can start a new local branch from the current master, cherry-pick the relevant commits from this branch, make sure that it works for you locally, then replace this github branch with your new local one. |
@mfasi Did you ever manage to recover this work on a new branch? As mentioned, we'd be very interested in getting this into Julia. |
Merged in #21184. |
Add the algorithm for
A^t
from Higham and Lin, 2013. See #5840.I split the old version of
logm
into several functions to reuse some of the code.