Let spdiagm preserve sparsity

[dkarrasch]

Closes #37174.

With this PR:

julia> y = sprand(10, 0.2)
10-element SparseVector{Float64,Int64} with 4 stored entries:
  [1 ]  =  0.290569
  [2 ]  =  0.454582
  [6 ]  =  0.578091
  [9 ]  =  0.822982

julia> spdiagm(0 => y)
10×10 SparseMatrixCSC{Float64,Int64} with 4 stored entries:
 0.290569   ⋅         ⋅    ⋅    ⋅    ⋅         ⋅    ⋅    ⋅         ⋅ 
  ⋅        0.454582   ⋅    ⋅    ⋅    ⋅         ⋅    ⋅    ⋅         ⋅ 
  ⋅         ⋅         ⋅    ⋅    ⋅    ⋅         ⋅    ⋅    ⋅         ⋅ 
  ⋅         ⋅         ⋅    ⋅    ⋅    ⋅         ⋅    ⋅    ⋅         ⋅ 
  ⋅         ⋅         ⋅    ⋅    ⋅    ⋅         ⋅    ⋅    ⋅         ⋅ 
  ⋅         ⋅         ⋅    ⋅    ⋅   0.578091   ⋅    ⋅    ⋅         ⋅ 
  ⋅         ⋅         ⋅    ⋅    ⋅    ⋅         ⋅    ⋅    ⋅         ⋅ 
  ⋅         ⋅         ⋅    ⋅    ⋅    ⋅         ⋅    ⋅    ⋅         ⋅ 
  ⋅         ⋅         ⋅    ⋅    ⋅    ⋅         ⋅    ⋅   0.822982   ⋅ 
  ⋅         ⋅         ⋅    ⋅    ⋅    ⋅         ⋅    ⋅    ⋅         ⋅ 

[barucden]

Have you considered using dispatching over isa? For example, the methods for determining the number of (non-zero) coefficients

_numel(vect::SparseVector) = nnz(vect)
_numel(vect::AbstractArray) = length(vect)

instead of

p.second isa AbstractSparseVector ? nnz(p.second) : length(p.second)

?

Imho, it is more readable and/or extensible.

[barucden]

AbstractSparseVector (unlike SparseVector) should be available since it is defined in abstractsparse.jl, which is included prior to sparsematrix.jl in SparseArrays.jl. I guess your second point is valid. I just think that dispatching is clearer than isa branching. Perhaps, someone more experienced will join the discussion.

[andreasnoack]

Which part of #37174 isn't addressed here?

[dkarrasch]

Which part of #37174 isn't addressed here?

The wish for a default spdiagm(v::AbstractVector) = spdiagm(0 => v). It's easy to add, actually, but I'm not sure if there is some hidden reason why that hasn't been done earlier.

[dkarrasch]

I included the convenience constructor. Originally, without much thinking, I was under the impression that this might be somewhat controversial, but the analogy to diagm is straightforward, and I don't think there is any conflict with existing functionality. Is this something we need to announce in NEWS? Seems pretty minor.

[andreasnoack]

The wish for a default spdiagm(v::AbstractVector) = spdiagm(0 => v).

Thanks. I missed that part. I guess the only reason not to have it would be to keep the code slightly simpler by only having a single interface for spdiagm. However, I suspect most people would intuitively try calling spdiagm(v::AbstractVector) as their first attempt so it might be friendly to include that method.

[ViralBShah]

Good stuff - Since @andreasnoack has already reviewed, I am happy (without my own review).

Off-topic - I wish we could pick better names (and even move sparse matrices out of stdlib sooner than 2.0)

[dkarrasch]

Something went horribly wrong with rebasing. I'll try again with #37684.