-
-
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
Add full(F::Factorization) for various kinds of factorization #9290
Conversation
@@ -108,6 +108,7 @@ convert{T}(::Type{Factorization{T}}, C::CholeskyPivoted) = convert(CholeskyPivot | |||
|
|||
full{T,S}(C::Cholesky{T,S,:U}) = C[:U]'C[:U] | |||
full{T,S}(C::Cholesky{T,S,:L}) = C[:L]*C[:L]' | |||
full{T}(F::CholeskyPivoted{T}) = (ip=invperm(F[:p]); (F[:L] * F[:U])[ip,ip]) |
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.
Type parameter is not necessary here. The type parameters are only included in the definitions for the usual Cholesky
in order to be able specify :U
and :L
.
Thanks for that. Please see the comments in the code. |
Include changes suggested by Andreas Noack
OK, adapted and squashed the PR. I am sorry that squashing outdates the diffs on github. |
## Can we determine the source/result is Real? This is not stored in the type Eigen | ||
full(F::Eigen) = F.vectors * Diagonal(F.values) / F.vectors | ||
|
||
full(F::Hessenberg) = (fq = full(F[:Q]); fq * F[:H] * fq') |
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.
You shouldn't construct the full Q
here. It is expensive. Use something like q = full(F[:Q]); (q * F[:H]) * q'
. The parentheses will avoid the bug I try to fix in #9170.
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 get the parentheses, but I don't really understand your remark about not reconstructing the full Q. Do you mean LAPACK.orghr!
is too expensive? Should I try to not extract the first row/column of Q?
@davidavdav I'm going through old issues and can see that this one fell under the radar. If you rebase then I'll merge. |
I have this almost rebased. |
This PR generalizes
full(::Cholesky)
to various other factorizations:CholeskyPivoted
,QR
,QRCompactWY
,QRPivoted
,Eigen
,Hessenberg
,Schur
,SVD
,LDLt
, andLU
. The idea is thatA = full(F::Factorization)
is the opposite ofF = factorize(A)
. I implemented the reconstruction for all factorizations, apart frombkfact
---somehow the.ipiv
encoding is too hard to understand for me, see http://www.nag.com/numeric/FL/nagdoc_fl22/pdf/F07/f07mdf.pdf , section 5.5.The patch includes code, tests and a line of documention.