LazyArrays.jl
LazyArrays.jl copied to clipboard
Move SparseArrays dependency to extension
This should help to resolve https://github.com/SciML/LinearSolve.jl/issues/573
Codecov Report
:x: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 0.00%. Comparing base (9dde192) to head (c3993e0).
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/lazyoperations.jl | 0.00% | 5 Missing :warning: |
| ext/LazyArraysSparseArraysExt.jl | 0.00% | 2 Missing :warning: |
:exclamation: There is a different number of reports uploaded between BASE (9dde192) and HEAD (c3993e0). Click for more details.
HEAD has 4 uploads less than BASE
Flag BASE (9dde192) HEAD (c3993e0) 6 2
Additional details and impacted files
@@ Coverage Diff @@
## master #365 +/- ##
==========================================
- Coverage 95.72% 0.00% -95.73%
==========================================
Files 17 18 +1
Lines 3208 3167 -41
==========================================
- Hits 3071 0 -3071
- Misses 137 3167 +3030
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
"my_" is a very bad naming convention. I have no idea what these are for!
Well the idea is to have some issparse and nnz methods which defaults to false in the case SparseArrays is not loaded, and is extended with the ones from SparseArrays in the extensions. Do you agree with the general approach ?
Instead of "my_" we could use "", "tentative" "local_" "provisional_" "preliminary_" ...
@j-fu I think local makes sense. Can you go for that?
I think even just beginning with_ would be fine, eg., LazyArrays._issparse.
But we would need a comment explaining the design
Review from Copilot/GPT5:
PR review: Move SparseArrays dependency to extension (#365)
Summary
- Converts SparseArrays from a hard dependency to an extension.
- Adds ext/LazyArraysSparseArraysExt.jl with methods that bridge to SparseArrays when available.
- Introduces internal fallbacks local_issparse and local_nnz and switches internal usage from issparse/nnz to these local functions.
- Updates Project.toml: moves SparseArrays to [weakdeps], adds LazyArraysSparseArraysExt in [extensions], adds compat entry and includes SparseArrays in [targets] test.
- Removes SparseArrays from the main using list in src/LazyArrays.jl.
Correctness and design
- The fallback definitions
- local_issparse(A) = false
- local_nnz(A) = prod(size(A)) preserve the previous logic in lazyoperations: nnz(X) is only used when X is sparse; otherwise prod(size(X)) is used. With the extension loaded, local_issparse(::AbstractArray) delegates to issparse and local_nnz(::AbstractSparseArray) to nnz, restoring the prior behavior without requiring SparseArrays globally.
- Dispatch is sensible: a broad Any fallback in-package and narrower AbstractArray/AbstractSparseArray methods in the extension. This avoids method piracy and keeps SparseArrays-specific code isolated.
- The changes in shuffle_algorithm correctly swap any(issparse, ...) and nnz(...) uses with local_issparse/local_nnz, maintaining semantics.
- Given LazyArrays already uses extensions for other packages, this is consistent with the package’s existing approach.
Nits and small suggestions
- src/LazyArrays.jl: consider removing the commented fragment in the using line to keep it clean:
- Current: using Base.Broadcast, LinearAlgebra, FillArrays, ArrayLayouts #, SparseArrays
- Suggested: using Base.Broadcast, LinearAlgebra, FillArrays, ArrayLayouts
- In lazyoperations.jl, local_nnz(A) = length(A) would be equivalent and a touch clearer than prod(size(A)) for AbstractArray inputs; either is fine.
- ext/LazyArraysSparseArraysExt.jl: a brief module comment noting the intent (bridge to SparseArrays for local_* functions) would mirror the helpful in-file comment in lazyoperations.jl.
Overall
- The implementation is minimal, well-scoped, and preserves behavior while removing the unconditional SparseArrays load. The approach looks solid and aligns with Julia’s extension mechanism. LGTM with the minor cleanups noted above.
If you bump the minor version (1.x) I can merge and tag