Skip to content

Conversation

j-fu
Copy link

@j-fu j-fu commented Feb 11, 2025

This should help to resolve SciML/LinearSolve.jl#573

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (9dde192) to head (c3993e0).

Files with missing lines Patch % Lines
src/lazyoperations.jl 0.00% 5 Missing ⚠️
ext/LazyArraysSparseArraysExt.jl 0.00% 2 Missing ⚠️

❗ 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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dlfivefifty
Copy link
Member

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

@j-fu
Copy link
Author

j-fu commented Feb 12, 2025

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_" ...

@ChrisRackauckas
Copy link
Member

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

@dlfivefifty
Copy link
Member

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

@dlfivefifty
Copy link
Member

But we would need a comment explaining the design

j-fu added 3 commits August 21, 2025 11:53
It is sufficient to define a method for `Any` in the package, and for `AbstractArray` in the
SparseArrays extension. Other dispatches are already implemented in SparseArrays.
@j-fu
Copy link
Author

j-fu commented Aug 21, 2025

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.

@dlfivefifty
Copy link
Member

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

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.

Circular dependency on 1.10
3 participants