Handle Statistics and SparseArrays as extensions

[dkarrasch]

I went ahead and just did it. The only "disturbing" thing is that kron method which is not an extension, strictly speaking. So it would be great to resolve it and make the dependencies weak.

[jishnub]

The Aqua formatting error on v1.6 should be fixed by #287

[jishnub]

Locally, I can get around the kron issue using
fillalgebra.jl:

abstract type KronStyle end
struct SparseKronStyle <: KronStyle end
struct UnknownKronStyle <: KronStyle end
KronStyle(A) = UnknownKronStyle()
kron(A::RectDiagonalFill, B::RectDiagonalFill) =
    maybesparsekron(KronStyle(A), KronStyle(B), A, B)

function maybesparsekron(::UnknownKronStyle, ::UnknownKronStyle, A::AbstractMatrix, B::AbstractMatrix)
    invoke(kron, Tuple{AbstractMatrix, AbstractMatrix}, A, B)
end

FillArraysSparseArraysExt.jl:

FillArrays.KronStyle(A::RectDiagonalFill) = FillArrays.SparseKronStyle()

function FillArrays.maybesparsekron(::SparseKronStyle, ::SparseKronStyle, A, B)
    kron(sparse(A), sparse(B))
end

I've updated the code in https://github.com/jishnub/FillArrays.jl/tree/kronextension

But, this is a more serious variant of type-piracy, where behavior is altered based on whether a library is loaded (and is potentially also a regression, as currently working code may lead to OOM errors if one doesn't load SparseArrays). Nonetheless, this may not be a widely used code path, so maybe such a change is acceptable?

[dlfivefifty]

The kron overload is a bit funny. It might be better to make a custom type if this is actually needed. Otherwise deprecate it and users can write kron(sparse(A), sparse(B)) instead?

[dkarrasch]

In fact, one should really go with a lazy Kronecker product, as in LazyArrays.jl, or Kronecker.jl, or LinearMaps.jl if you don't need the result to subtype AbstractMatrix. With the "vec trick" and fast multiplication, this should be superior in most applications.

[codecov]

Codecov Report

Merging #286 (7c7a333) into master (41715aa) will decrease coverage by 99.89%.
Report is 3 commits behind head on master.
The diff coverage is 0.00%.

❗ Current head 7c7a333 differs from pull request most recent head 6147c9e. Consider uploading reports for the commit 6147c9e to get more accurate results

@@            Coverage Diff             @@
##           master    #286       +/-   ##
==========================================
- Coverage   99.88%   0.00%   -99.89%     
==========================================
  Files           5       7        +2     
  Lines         851     868       +17     
==========================================
- Hits          850       0      -850     
- Misses          1     868      +867     

Files Changed	Coverage Δ
ext/FillArraysSparseArraysExt.jl	0.00% <0.00%> (ø)
ext/FillArraysStatisticsExt.jl	0.00% <0.00%> (ø)
src/FillArrays.jl	0.00% <0.00%> (-99.74%)	⬇️
src/fillalgebra.jl	0.00% <0.00%> (-100.00%)	⬇️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[dkarrasch]

How do we proceed here? Did you mean to release one version with a deprecation only, and then a subsequent version with the Pkg extension stuff like here? An alternative would be to release v2, which may take a while to propagate through the ecosystem.

[dlfivefifty]

Technically for SEMVAR: if we wanted to drop support we could deprecate in a 1.x release and then remove in a 2.0 release. But then this means there's no actual benefit until 2.0!

BUT: I take the view that a change is non-breaking if it doesn't break anything. @jishnub why was kron added? Do you need it or could you use kron(sparse(A), sparse(B))?

[jishnub]

kron was added because there was an issue about this: #265. I don't have strong opinions either way.

cc: @amilsted for your use case, would you be ok with this change?

[dlfivefifty]

How about this: just have kron throw an error when SparseArrays.jl is not loaded? I.e.

# in FillArrays.jl
kron(A::RectDiagonalFill, B::RectDiagonalFill) = kron_fill(A, B)
kron_fill(A, B) = error("Load SparseArrays.jl")

# in FillArraysSparseArraysExt.jl
kron_fill(A::RectDiagonalFill, B::RectDiagonalFill) = kron(sparse(A), sparse(B))

[jishnub]

This would be breaking, though. The hack-y solution that I had presented above was trying to work around this breakage.

[dlfivefifty]

It's not really "breaking" as downstream one just has to do using SparseArrays. I'd be shocked if there's any extant code that's impacted

[jishnub]

Alright, perhaps we may go ahead with making this error without SparseArrays.

Edit: the current design in this PR seems fine to me.

julia> kron(E, E)
4×4 Matrix{Float64}:
 1.0  0.0  0.0  0.0
 0.0  1.0  0.0  0.0
 0.0  0.0  1.0  0.0
 0.0  0.0  0.0  1.0

julia> using SparseArrays

julia> kron(E, E)
4×4 SparseMatrixCSC{Float64, Int64} with 4 stored entries:
 1.0   ⋅    ⋅    ⋅ 
  ⋅   1.0   ⋅    ⋅ 
  ⋅    ⋅   1.0   ⋅ 
  ⋅    ⋅    ⋅   1.0

The method is deprecated anyway, and if this impacts anyone, they may be warned against using this.

[dkarrasch]

Oh, I missed your update @jishnub. If everyone is happy with the last but one commit, I can revert the "error or load" commit.

[jishnub]

I think this should be good to merge?

[amilsted]

Do I understand right that kron will now be deprecated for these arrays regardless of whether SparseArrays is loaded? That seems fair enough. It's a bit weird that the extension package changes the output type of kron, but if it's going away I suppose it's not a big deal.

[jishnub]

Yes, that's the status quo