JuliaStats / PDMats.jl

Uniform Interface for positive definite matrices of various structures
Other
104 stars 43 forks source link

Unify `PDMat` and `PDSparseMat` + move SparseArrays support to an extension #188

Open devmotion opened 11 months ago

devmotion commented 11 months ago

This PR unifies PDMat and PDSparseMat and makes PDSparseMat an (unexported) alias of PDMat. This makes it also possible to move support for SparseArrays to an extension, without any workarounds such as non-functional types in the main package (see #174).

Initially, PDMat and PDSparseMat only supported Matrix{Float64} and SparseMatrixCSC{Float64} matrices. This restriction was lifted in https://github.com/JuliaStats/PDMats.jl/pull/37 but the separate types with support for Cholesky and CHOLMOD.Factors were kept.

Due to the refactoring of SuiteSparse and SparseArrays (it was integrated in SparseArrays in Julia 1.9), on Julia >= 1.9 only the SparseArrays dependency is needed and SuiteSparse only loads functionality from SparseArrays (https://github.com/JuliaSparse/SuiteSparse.jl/blob/e8285dd13a6d5b5cf52d8124793fc4d622d07554/src/SuiteSparse.jl). It seemed inconvenient to demand that users on Julia >= 1.9 load both SparseArrays and SuiteSparse for the sparse extension, and hence I decided to let the extension only depend on SparseArrays. IIUC, however, this means that we can't support Julia < 1.9 anymore since there SparseArrays does not contain the required CHOLMOD functionality. @dkarrasch, is this correct?

Since this means we drop support for the current LTS version, my suggestion would be to backport bugfixes to older Julia versions (at least >= 1.6) until another Julia version >= 1.9 becomes the new LTS version.

Edit: The PR keeps support for Julia 1.0-, I found a workaround: https://github.com/JuliaStats/PDMats.jl/pull/188#issuecomment-1746645655

Fixes #174.

codecov-commenter commented 11 months ago

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (b85c8ac) 91.61% compared to head (6678dd3) 92.18%.

Files Patch % Lines
ext/PDMatsSparseArraysExt/pdsparsemat.jl 94.66% 4 Missing :warning:
src/pdmat.jl 97.67% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #188 +/- ## ========================================== + Coverage 91.61% 92.18% +0.56% ========================================== Files 9 11 +2 Lines 680 640 -40 ========================================== - Hits 623 590 -33 + Misses 57 50 -7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

devmotion commented 11 months ago

Perhaps we can make this work even for older Julia versions by using SuiteSparse or SparseArrays.SuiteSparse in the extension? On v1.9+, this should be a no-op, but on older versions (without Pkg extensions) this will just get loaded by including the extension code files. I'm not sure though if that would require "version-dependent" Project.toml files or other hassle.

AFAICT on older Julia versions we would have to keep both SparseArrays and SuiteSparse as dependencies. However, if only Julia >= 1.9 the extension only depends on SparseArrays, then SuiteSparse would still be a regular dependency and since it depends on SparseArrays the extension would be loaded unconditionally. Alternatively, the extension could depend on both SparseArrays and SuiteSparse (or only SuiteSparse) - but then users would have to load both SparseArrays and SuiteSparse (or only SuiteSparse). Both alternatives seem suboptimal.

However, I just thought of another option that I will try later: Making SparseArrays and SuiteSparse both a weak dependency but letting the extension only depend on SparseArrays.

devmotion commented 11 months ago

Seems to work 🙂 On Julia 1.0 and 1.6, PDMats depends on SparseArrays and SuiteSparse (https://github.com/JuliaStats/PDMats.jl/actions/runs/6404900221/job/17386347115?pr=188#step:5:36, https://github.com/JuliaStats/PDMats.jl/actions/runs/6404900221/job/17386347711?pr=188#step:5:27) and on Julia 1.9 and nightly neither SparseArrays nor SuiteSparse are dependencies (https://github.com/JuliaStats/PDMats.jl/actions/runs/6404900221/job/17386348265?pr=188#step:5:24, https://github.com/JuliaStats/PDMats.jl/actions/runs/6404900221/job/17386348761?pr=188#step:5:24) and the tests pass successfully with installing and loading only SparseArrays.

devmotion commented 10 months ago

I'll update the PR once the other PRs (non-breaking bugfixes and possibly a new feature) are merged.

timholy commented 7 months ago

Bump

devmotion commented 7 months ago

The PR is the result of some discussions I had with @andreasnoack about PDSparseMat and the SparseArrays dependency of PDMats, so I'd prefer to wait for his review.

timholy commented 7 months ago

Right, I wasn't bumping you 🙂