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

Add a sparse `ev_view` for `SymTridiagonal` backed by sparse arrays

Open mcognetta opened this issue 3 years ago • 7 comments

This is a fix for https://github.com/JuliaLang/julia/issues/46355, where SymTridiagonal matrices backed by sparse arrays couldn't be summed/subtracted/etc. The root cause is that adding @views of SparseArrays produces a dense array.

https://github.com/JuliaLang/julia/blob/ec98087cbf19bac26f6be05df9282746b0cffe78/stdlib/LinearAlgebra/src/tridiag.jl#L207

https://github.com/JuliaLang/julia/blob/ec98087cbf19bac26f6be05df9282746b0cffe78/stdlib/LinearAlgebra/src/LinearAlgebra.jl#L449-L453

julia> z = @view sparse([1, 2, 3])[1:3]
3-element view(::SparseVector{Int64, Int64}, 1:3) with eltype Int64:
 1
 2
 3

julia> z + z
3-element Vector{Int64}:
 2
 4
 6

This view usage was added in https://github.com/JuliaLang/julia/pull/42472 to deal with an implementation detail of SymTridiagonals where the off-diagonal has an additional element that should usually be ignored. A view was used to access only the actual elements of the off-diagonal without copying. This PR makes it so that sparse arrays just use a regular (copying) slice, until a better fix is found (I am not sure if there has been any discussion about the sparse @view sum thing).

mcognetta avatar Aug 16 '22 14:08 mcognetta

wouldn't it be just better to fix SparseVectorView + SparseVectorView addition? @ViralBShah yet another argument for AbstractCompressedSparseVector?

SobhanMP avatar Aug 16 '22 21:08 SobhanMP

Yes, it would be better, if that is something that should actually be done. I don't know if there are any weird considerations that need to be addressed when summing two views though, so I am fixing it this way for now.

mcognetta avatar Aug 17 '22 05:08 mcognetta

Yeah we should just fix the views.

ViralBShah avatar Aug 17 '22 14:08 ViralBShah

@ViralBShah so do i extract the AbstractCompressedVector from #201 and merge it? We can make the default for all operations on AbstractCompressedVectors return SparseVectors

SobhanMP avatar Aug 17 '22 20:08 SobhanMP

@Wimmerer Any thoughts on your part?

ViralBShah avatar Aug 17 '22 22:08 ViralBShah

@ViralBShah so do i extract the AbstractCompressedVector from #201 and merge it? We can make the default for all operations on AbstractCompressedVectors return SparseVectors

@SobhanMP Yes.

rayegun avatar Aug 21 '22 00:08 rayegun

Codecov Report

Merging #225 (984209f) into main (f51b64c) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
+ Coverage   92.11%   92.12%   +0.01%     
==========================================
  Files          11       11              
  Lines        7203     7204       +1     
==========================================
+ Hits         6635     6637       +2     
+ Misses        568      567       -1     
Impacted Files Coverage Δ
src/SparseArrays.jl 100.00% <100.00%> (ø)
src/linalg.jl 84.58% <0.00%> (+0.08%) :arrow_up:

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

codecov-commenter avatar Aug 21 '22 00:08 codecov-commenter