-
Notifications
You must be signed in to change notification settings - Fork 43
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
Avoid copy in chol_lower
#144
Conversation
Side note: |
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 you can just make this chol_lower(a::Matrix) = cholesky(Hermitian(a, :L)).L
.
I agree that CholType
is redundant but please make that change in a separate PR.
This comment has been minimized.
This comment has been minimized.
Writing it in terms of |
No. The default is upper julia> cholesky(A).uplo
'U': ASCII/Unicode U+0055 (category Lu: Letter, uppercase)
julia> cholesky(Hermitian(A, :L)).uplo
'L': ASCII/Unicode U+004C (category Lu: Letter, uppercase) |
Ok, not literally the same code, but julia> using LinearAlgebra, PDMats
julia> A=rand(100,100); C=A'A;
julia> @allocated cholesky(Hermitian(C, :L)).L; # compile
julia> @allocated cholesky(Hermitian(C, :L)).L
80208
julia> @allocated cholesky(C).U'; # compile
julia> @allocated cholesky(C).U'
80192
julia> @allocated PDMats.chol_lower(cholesky(C)); # compile
julia> @allocated PDMats.chol_lower(cholesky(C))
80192 |
It's just an artifact of the not wrapping it in a function julia> chol_lower_an(A::Matrix) = cholesky(Hermitian(A, :L)).L;
julia> @allocated chol_lower_an(A);
julia> @allocated chol_lower_an(A)
80144 It's also best to avoid wrapper types when possible. |
Ah, thanks for pointing that out.
Could you expand on what you mean ? |
Sure. The more types layers you have the less likely it is that a specific method has been written for it and you risk hitting a less optimized fallback. The compiler can also sometimes completely remove these wrapper types but I believe that the more layers you have, the harder it is for the compiler to do so. |
Ok, so do I understand correctly, you would prefer -chol_lower(a::Matrix) = chol_lower(cholesky(a))
+chol_lower(a::Matrix) = cholesky(Hermitian(a, :L)).L because the former returns a |
That is correct |
Thanks, that makes sense. I've changed it accordingly:) |
Codecov Report
@@ Coverage Diff @@
## master #144 +/- ##
=======================================
Coverage 88.62% 88.62%
=======================================
Files 8 8
Lines 378 378
=======================================
Hits 335 335
Misses 43 43
Continue to review full report at Codecov.
|
Downside of merging it in with |
Oh no. Do you have an example? |
Yes, it turns out to be a Zygote issue, I've made a PR to fix it here: FluxML/Zygote.jl#1114 |
@andreasnoack looks like the Zygote issue might be a bit more complex to resolve; how about (at least temporarily) changing it from |
Given that we have this restriction Line 36 in 635a957
|
Fixes #143