-
Notifications
You must be signed in to change notification settings - Fork 53
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
implement in-place ldiv!
for Cholesky factorization
#547
Conversation
7efa44e
to
bf39182
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #547 +/- ##
==========================================
+ Coverage 84.07% 84.16% +0.09%
==========================================
Files 12 12
Lines 9067 9106 +39
==========================================
+ Hits 7623 7664 +41
+ Misses 1444 1442 -2 ☔ View full report in Codecov by Sentry. |
@ViralBShah @dkarrasch @rayegun Does any of you have some time to review this PR? |
I will take a look today. Also hoping someone else can in addition. |
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.
With the exception that I'm not clear if Dense inputs are tested sgtm. Will merge once that has an answer
This looks straightforward. Merging. The PR just looked much bigger because of all the whitespace changes. I'd like to hold this PR until I resolve a couple of docs issues that need to get backported into 1.11 - which I hope will be in a day or two. |
Thanks a lot! I'm sorry for the confusing with the whitespace changes - VSCode did all that for me and I didn't notice before |
Is it possible to add a docstring? |
It does not differ from the standard |
Yeah good point. Not necessary. |
Thanks a lot for your reviews! Do you know which Julia version will include this patch? |
It will go on Julia master when we bump SparseArrays.jl - happens every few days, and then go into 1.12. |
Thanks! |
I have implemented an in-place
ldiv!
for Cholesky factorizations of sparse matrices.Closes #319
Closes #275
Closes #123
Xref https://discourse.julialang.org/t/is-there-no-ldiv-for-choleski-factorization-of-a-sparse-matrix/114806
Caveats
The method works locally but is not as elegant as the UMFPack/LU version or the other code in the CHOLMOD wrapper:
cholmod_dense
structs.Dense
wrappers in the Julia CHOLMOD module allocate new arrays.Thus, the code looks a bit different from the remaining code in the file.
The current method still allocates some memory on my system since we do not provide the workspace vectors
Handle_Y
andHandle_E
. We could store them in theCHOLMOD.Factor
struct. I tried that, but they were stillC_NULL
after the call tosolve2
(when initialized withC_NULL
). Thus, re-using them is not working as I understand the docs. Thus, I have not added this complexity to this PR.Maybe related: DrTimothyAldenDavis/SuiteSparse#45
Questions
Shall we announce this somewhere in some form of NEWS.md so that people can adopt to this change?