PDMats.jl icon indicating copy to clipboard operation
PDMats.jl copied to clipboard

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

Open devmotion opened this issue 2 years ago • 8 comments

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.

devmotion avatar Oct 04 '23 07:10 devmotion

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
@@            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.

codecov-commenter avatar Oct 04 '23 07:10 codecov-commenter

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 avatar Oct 04 '23 09:10 devmotion

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 avatar Oct 04 '23 11:10 devmotion

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

devmotion avatar Oct 17 '23 07:10 devmotion

Bump

timholy avatar Jan 12 '24 13:01 timholy

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.

devmotion avatar Jan 13 '24 22:01 devmotion

Right, I wasn't bumping you 🙂

timholy avatar Jan 13 '24 22:01 timholy

Now that the lower compat has been raised to v1.10, this would be worthwhile to revisit and finish. I don't have current loading/latency times, but at the time this would have helped avoiding to load SparseArrays.jl unconditionally and therefore help all the dependent packages to keep loading times low(er).

dkarrasch avatar Feb 21 '25 09:02 dkarrasch