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

implement in-place ldiv! for Cholesky factorization #547

Merged
merged 8 commits into from
Aug 7, 2024

Conversation

ranocha
Copy link
Contributor

@ranocha ranocha commented Jul 27, 2024

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:

  • UMFPack accepts arrays/pointers directly whereas CHOLMOD uses cholmod_dense structs.
  • The 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 and Handle_E. We could store them in the CHOLMOD.Factor struct. I tried that, but they were still C_NULL after the call to solve2 (when initialized with C_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?

@ranocha ranocha marked this pull request as draft July 27, 2024 13:50
@ranocha ranocha marked this pull request as ready for review July 27, 2024 14:34
Copy link

codecov bot commented Jul 27, 2024

Codecov Report

Attention: Patch coverage is 84.78261% with 7 lines in your changes missing coverage. Please review.

Project coverage is 84.16%. Comparing base (4141e8a) to head (fcf34fb).
Report is 1 commits behind head on main.

Files Patch % Lines
src/solvers/cholmod.jl 84.78% 7 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@ViralBShah
Copy link
Member

cc @dkarrasch @rayegun

@ranocha
Copy link
Contributor Author

ranocha commented Aug 5, 2024

@ViralBShah @dkarrasch @rayegun Does any of you have some time to review this PR?

@ViralBShah
Copy link
Member

I will take a look today. Also hoping someone else can in addition.

Copy link
Member

@rayegun rayegun left a 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

test/cholmod.jl Show resolved Hide resolved
@ViralBShah
Copy link
Member

ViralBShah commented Aug 5, 2024

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.

@ranocha
Copy link
Contributor Author

ranocha commented Aug 5, 2024

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

@ViralBShah
Copy link
Member

Is it possible to add a docstring?

@ranocha
Copy link
Contributor Author

ranocha commented Aug 6, 2024

It does not differ from the standard ldiv! behavior, so I didn't add a docstring - many people expect it to just work as discussed in the issues linked above. Would you prefer an explicit docstring? If so, I can add one

@ViralBShah
Copy link
Member

Yeah good point. Not necessary.

@ViralBShah ViralBShah merged commit b8a13ef into JuliaSparse:main Aug 7, 2024
10 checks passed
@ranocha
Copy link
Contributor Author

ranocha commented Aug 7, 2024

Thanks a lot for your reviews! Do you know which Julia version will include this patch?

@ViralBShah
Copy link
Member

ViralBShah commented Aug 7, 2024

It will go on Julia master when we bump SparseArrays.jl - happens every few days, and then go into 1.12.

@ranocha
Copy link
Contributor Author

ranocha commented Aug 7, 2024

Thanks!

@ranocha ranocha deleted the hr/ldiv_cholesky branch August 7, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants