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

WIP: Improve accuracy of ^ for ill-conditioned matrices #12584

Closed
wants to merge 1,117 commits into from

Conversation

mfasi
Copy link
Contributor

@mfasi mfasi commented Aug 12, 2015

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.

@mfasi mfasi changed the title Improve ^ for matrices RFC: Improve ^ for matrices Aug 12, 2015
@ivarne ivarne added the linear algebra Linear algebra label Aug 12, 2015
@StefanKarpinski
Copy link
Member

"Improve" is a little vague. Does this change behavior or just performance? Some explanation and examples of the difference would be nice.

@mfasi
Copy link
Contributor Author

mfasi commented Aug 12, 2015

@StefanKarpinski You're right, an explanation might help. The implementation that we have now in base is not reliable for general matrices because it fails when the eigenvectors of the matrix are ill-conditioned. A classical example is the comparison of the (relative) residuals for

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 master I get

julia> norm((A^(1/2))^2 - A) / norm(A)
0.6180339887498948

and on powm

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.

@pao pao changed the title RFC: Improve ^ for matrices RFC: Improve accuracy of ^ for ill-conditioned matrices Aug 12, 2015
warn("Matrix with nonpositive real eigenvalues, a nonprincipal matrix power will be returned.")
end

if isreal(A) && ~np_real_eigs
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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

  1. Mimic scalar function and return DomainError
  2. 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.

@mfasi
Copy link
Contributor Author

mfasi commented Aug 14, 2015

@stevengj Thank you for checking the patch thoroughly. I addressed most of the points you raised, still to do:

  • add tests
  • add documentation
  • push cond(A) after the Schur decomposition

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).

@mfasi mfasi changed the title RFC: Improve accuracy of ^ for ill-conditioned matrices WIP: Improve accuracy of ^ for ill-conditioned matrices Aug 14, 2015
@mfasi
Copy link
Contributor Author

mfasi commented Aug 17, 2015

I managed to push the use of cond(A) after the Schur factorization. I added inline documentation and tests as well.

@@ -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)
Copy link
Member

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.

Copy link
Contributor Author

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.

Sacha0 and others added 20 commits September 5, 2016 19:18
…ons and deprecation of `@vectorize_1arg` and `@vectorize_2arg` themselves. Also add cross-referenes between max and maximum, and also between min and minimum.
…18342)

* don't throw DomainError for negative integer powers of ±1

* fix replutil test that was expecting 1^(-1) to fail
@stevengj
Copy link
Member

stevengj commented Sep 5, 2016

Uh oh, looks like something went terribly wrong with a rebase...

@mfasi
Copy link
Contributor Author

mfasi commented Sep 5, 2016

@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

@alyst
Copy link
Contributor

alyst commented Sep 5, 2016

@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.

@andreasnoack
Copy link
Member

@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.

@stevengj
Copy link
Member

stevengj commented May 1, 2017

Merged in #21184.

@stevengj stevengj closed this May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.