-
-
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
RFC:QR decomposition in julia #5526
Conversation
for i = 1:mA | ||
νAi = A[i,k] | ||
for j = k+1:mQ | ||
νAi += Q.factors[j,k]*A[i,j] |
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 am not sure if this gets hoisted by the compiler, but you may want to have Qfactors = Q.factors
outside the loops.
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.
@Viral It made a significant difference in speed to do this.
This is amazing stuff, but also quite a big PR, making it a bit difficult to review. @jiahao should take a look as well. |
This looks correct upon a first reading. However, I'm AFC until later in the afternoon. |
@ViralBShah Yes it got too big. I didn't anticipate the consequences of the changes to the tests. I forgot that I have changed the names of the QR types. There are three now: the usual unpivoted ( I should also add that there is a lot of rank one updating written out in loops in the QR code. When we have fast array views, it should be possible to remove many code lines. |
Not directly related, but will this make it easy to implement functionality like in qrupdate? |
For fast |
@mlubin I don't know what it will take to implement updating. I tried to look at your link, but I think the only documentation is the source code. |
@@ -607,7 +607,6 @@ export | |||
eigvecs, | |||
expm, | |||
eye, | |||
factorize!, |
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.
Good riddance.
@jiahao I have updated the pull request with your suggestions so far. |
Thanks. I guess I have more bedtime reading tonight. |
One more update of the code. The direct test against LAPACK was useful and caught a conjugation error. |
@jiahao In order to get the same result for the julia LAPACK LU I had to switch to the BLAS style square root free absolute value |
Just want to point out that your observation can be rephrased (with slight mathematical abuse) as LAPACK using the |
Since |
I've read through the core QR algorithms twice now and they seem correct. So if no one else has any objections, I think you can declare victory as you see fit. |
…y reflectors. Add Float16 and BigFloat to tests and test promotion. Cleaup promotion in LinAlg. Avoid promotion in mutating ! functions. Make Symmetric, Hermitian and QRs immutable. Make thin a keyword argument in svdfact. Remove cond argument in sqrtm.
RFC:QR decomposition in julia
👯 |
This pull request started as a QR decomposition in julia such that e.g.
Matrix{BigFloat}
can be decomposed. However, while adding the tests I also decided to go through more combinations of types which revealed some problems and I have therefore changed a lot of the promotion rules. This request should therefore also fix JuliaLang/LinearAlgebra.jl#54. I decided that the!
versions shouldn't promote but assume that it has called with a reasonable type.The option for returning the condition number in
sqrtm
was broken and undocumented so I decided to remove it. There was also a few bugs in leading dimension calculations inlapack.jl
and more the factorization and special matrix types have been changed fromtype
toimmutable
.I am aware that this pr includes the
balance
option toeigfact
, but I expect that the balance pull request will be merged very soon.