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

Account for pivoting in sparse Cholesky decompositions #175

Merged
merged 4 commits into from
Oct 2, 2023

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Jul 31, 2023

As explained in the docstring, one should be careful when working with Cholesky decompositions of sparse arrays since the property :L does not take into account the pivoting - in contrast to :PtL which also includes the necessary permutations.

This PR changes chol_lower and chol_upper for such sparse Cholesky decompositions such that pivoting is taken into account. Unfortunately, since SparseArrays does not support multiplication of such FactorComponents, code that multiplies the output of chol_lower/chol_upper requires a different, explicitly constructed sparse equivalent of these factors. Alternatively, one could define internal functions for multiplying the output of chol_lower/chol_upper, let them fall back to * by default, and add special implementations for these sparse factors. I've not done this for now because I was not completely sure whether the sparse special case justifies such changes to the (internal) API.

Additionally, the PR adds a seemingly missing definition of chol_upper(::Matrix) and fixes a few test errors in test/pdmtypes.jl and test/specialarrays.jl caused by upstream bugs.

Fixes #120.

Edit: I suggest merging #180 first. That PR fixes the test issues on the master branch properly whereas this PR here only contains a workaround.

@devmotion
Copy link
Member Author

Test failures on Julia 1.6 seem to be caused by an upstream issue in ArrayLayouts: JuliaLinearAlgebra/ArrayLayouts.jl#161

@dkarrasch
Copy link
Member

Would it make sense to make an extra package for the PDSparseMat stuff? According to juliahub.com, PDMats.jl has 1k+ dependents, which, starting from v1.10, will all need to load SparseArrays.jl (no longer in the sysimage) by default. That is a heavy load for the ecosystem, which would be good to avoid, if possible.

@devmotion
Copy link
Member Author

devmotion commented Aug 4, 2023

Maybe, I'm not sure. It would require a breaking change though so it's also unclear whether/how quickly the package ecosystem would adopt the breaking release.

In any case, I think this consideration is unrelated to the PR. The PR just fixes bugs in the current release of PDMats.

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
src/chol.jl 100.00%
src/pdsparsemat.jl 100.00%

📢 Thoughts on this report? Let us know!.

@devmotion
Copy link
Member Author

This PR is ready for review as well @andreasnoack 🙂

@devmotion devmotion merged commit 5ca3316 into master Oct 2, 2023
11 checks passed
@devmotion devmotion deleted the dw/sparse_pivot branch October 2, 2023 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No pivoting for CHOLMOD factors
4 participants