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

Move SparseArrays dependency to extension

Open j-fu opened this issue 10 months ago • 3 comments

This should help to resolve https://github.com/SciML/LinearSolve.jl/issues/573

j-fu avatar Feb 11 '25 22:02 j-fu

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.

codecov[bot] avatar Feb 12 '25 10:02 codecov[bot]

"my_" is a very bad naming convention. I have no idea what these are for!

dlfivefifty avatar Feb 12 '25 14:02 dlfivefifty

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 avatar Feb 12 '25 17:02 j-fu

@j-fu I think local makes sense. Can you go for that?

ChrisRackauckas avatar Aug 20 '25 21:08 ChrisRackauckas

I think even just beginning with_ would be fine, eg., LazyArrays._issparse.

dlfivefifty avatar Aug 21 '25 09:08 dlfivefifty

But we would need a comment explaining the design

dlfivefifty avatar Aug 21 '25 09:08 dlfivefifty

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.

j-fu avatar Aug 21 '25 10:08 j-fu

If you bump the minor version (1.x) I can merge and tag

dlfivefifty avatar Aug 21 '25 14:08 dlfivefifty