Move SparseArrays to an extension

[jishnub]

No description provided.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.19% 🎉

Comparison is base (14721f1) 87.08% compared to head (968fcf8) 87.27%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage   87.08%   87.27%   +0.19%     
==========================================
  Files          10       11       +1     
  Lines        1773     1784      +11     
==========================================
+ Hits         1544     1557      +13     
+ Misses        229      227       -2     

Files Changed	Coverage Δ
ext/ArrayLayoutsSparseArraysExt.jl	100.00% <100.00%> (ø)
src/ArrayLayouts.jl	84.12% <100.00%> (+1.33%)	⬆️
src/ldiv.jl	90.40% <100.00%> (+0.07%)	⬆️
src/memorylayout.jl	84.58% <100.00%> (+0.05%)	⬆️
src/mul.jl	84.86% <100.00%> (+0.14%)	⬆️
src/muladd.jl	75.29% <100.00%> (ø)

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

[dlfivefifty]

Could you add tests for those two lines? (I realise they weren't covered before but just to make sure it isn't breaking something that was working)

[dkarrasch]

Could it be that the dependency on FillArrays.jl is also weak? From what I see, it looks like whenever there is a call to it, FillArray types are already involved in the signature. Or, conversely, is it possible to ever hit any of those methods without loading FillArrays.jl, constructing some Fill* object, and then get in touch with ArrayLayouts.jl?

[jishnub]

I think it's almost a weak dep, but I found that FillArrays.axes_print_matrix_row is used in Base.print_matrix_row without any reference to AbstractFill types

[dkarrasch]

How many methods exist for axes_print_matrix_row in FillArrays.jl? Do we need to dispatch, or could we simply have a copy of that function here? If it doesn't dispatch on FillArrays.jl types, it should work for ArrayLayouts.jl, using Base or ArrayLayouts.jl-owned methods and types, shouldn't it?

[dkarrasch]

Actually, this seems to be an easy case. FillArrays.jl defines

if VERSION < v"1.8-"
    axes_print_matrix_row(lay, io, X, A, i, cols, sep) =
        Base.invoke(Base.print_matrix_row, Tuple{IO,AbstractVecOrMat,Vector,Integer,AbstractVector,AbstractString},
                    io, X, A, i, cols, sep)
else
    axes_print_matrix_row(lay, io, X, A, i, cols, sep, idxlast::Integer=last(axes(X, 2))) =
        Base.invoke(Base.print_matrix_row, Tuple{IO,AbstractVecOrMat,Vector,Integer,AbstractVector,AbstractString,Integer},
                    io, X, A, i, cols, sep, idxlast)
end

so it is just an indirection, without reference to FillArrays's methods or types, which, on top, can be removed altogether once the lower bound is lifted to some version higher than v1.8. So axes_print_matrix_row could be copied here, and the import removed. If that solves the dependency issue already, that would be awesome!

[jishnub]

The patch coverage is fixed now

[dkarrasch]

So, shall we also make FillArrays.jl a weak dependency here? Can I help with anything?

[jishnub]

FillArrays also uses axes_print_matrix_row in print_matrix_row. Would it be possible to resolve that one as well?
https://github.com/JuliaArrays/FillArrays.jl/blob/a718e2f66b9a4ddb29a5bd8539cdf127b2280c22/src/FillArrays.jl#L756-L764

[dkarrasch]

Sure, you just keep things internal, don't import them from each other, and have copies in each of the packages. And, TBH, that's what they are: internal functions. One could in fact get rid of them completely by having version-dependent versions of the print functions, which call the Base functions directly.

[jishnub]

You are welcome to submit a PR with the changes

[jishnub]

@dlfivefifty should be good to merge? Coverage is fixed now