Bugfix sparse_mul! handling of beta

[lkdvos]

This fixes #17 using the method discussed there, which pre-multiplies the output storage with beta.

Some questions I had, which relate to some design choices:

• If we are not assuming that the unstored values are zeros, I cannot "empty" the destination storage, even with beta = 0. As a result, the current implementation will not discard stored zeros. Maybe we want a mechanism to be able to control that?
• I looked into using map_stored!, but it was unclear to me whether using a_dest both as input and output is something that is allowed by the API. I know the Base.map! has a warning for this, and so we should probably decide whether the API supports that or not.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.40%. Comparing base (68399d6) to head (45f680f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #18      +/-   ##
==========================================
+ Coverage   63.13%   63.40%   +0.26%     
==========================================
  Files           6        6              
  Lines         274      276       +2     
==========================================
+ Hits          173      175       +2     
  Misses        101      101              

Flag	Coverage Δ
docs	63.27% <100.00%> (+0.26%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[mtfishman]

* If we are not assuming that the unstored values are zeros, I cannot "empty" the destination storage, even with `beta = 0`. As a result, the current implementation will not discard stored zeros. Maybe we want a mechanism to be able to control that?

It's a good question about whether we are assuming unstored values are zeros or not, and maybe that is not handled in a consistent way right now. Even though you can customize getunstoredindex to output values other than zero, a lot of code still assumes the unstored value is some kind of zero (either numerical zero or a zero array). Probably we should formalize that a bit better and make it clear what operations make that assumption. Maybe it even warrants having different kinds of sparse arrays, where SparseArrayDOK does assume the unstored values are zeros, and then we can have a GenericSparseArrayDOK that doesn't make that assumption. My thinking on that was basically that as long as we are clear on what functions assume unstored values are zeros, it is "buyer beware" and you are free to construct SparseArrayDOK with other unstored values but some operations like matrix multiplication won't make sense (if you have something like an array of strings, algebra operations like matrix multiplication don't make much sense anyway).

In terms of emptying the destination storage, I actually thought that was working, but I guess not:

julia> using SparseArraysBase

julia> a = SparseArrayDOK{Float64}(2, 2)
2×2 SparseMatrixDOK{Float64, typeof(SparseArraysBase.default_getunstoredindex)}:
 ⋅  ⋅
 ⋅  ⋅

julia> a[1, 1] = 1
1

julia> a .*= 0
2×2 SparseMatrixDOK{Float64, typeof(SparseArraysBase.default_getunstoredindex)}:
 0.0  ⋅
  ⋅   ⋅

julia> a[1, 1] = 1
1

julia> map!(Returns(0), a, a)
2×2 SparseMatrixDOK{Float64, typeof(SparseArraysBase.default_getunstoredindex)}:
 0.0  ⋅
  ⋅   ⋅

julia> a[1, 1] = 1
1

julia> fill!(a, 0)
2×2 SparseMatrixDOK{Float64, typeof(SparseArraysBase.default_getunstoredindex)}:
 0.0  ⋅
  ⋅   ⋅

julia> a[1, 1] = 1
1

The only one that works is:

julia> SparseArraysBase.zero!(a)
2×2 SparseMatrixDOK{Float64, typeof(SparseArraysBase.default_getunstoredindex)}:
 ⋅  ⋅
 ⋅  ⋅

At some point I thought I had a codepath in fill, map, and broadcast that checked if it is filling with zero and in that case it would call SparseArraysBase.zero! but maybe that got removed during one of the refactors.

* I looked into using `map_stored!`, but it was unclear to me whether using `a_dest` both as input and output is something that is allowed by the API. I know the `Base.map!` has a warning for this, and so we should probably decide whether the API supports that or not.

Good question, I didn't think too much about aliasing in the map operations. More broadly, we should discuss what we want the API to be for operating on just the stored values, in general I have been trying to push that down to an implementation detail so that higher level code and users don't have to think about that, since that can go wrong pretty easily (since it can depend on the ordering the data is stored if you have operations with multiple arrays). But maybe it would be good to make map_stored! "more official", or users should use things like map!(x -> 2x, storedvalues(a), storedvalues(a)) or storedvalues(a) .*= 2, assuming the aliasing is handled properly. For those kinds of cases, though, I was hoping to make the generic functions map!(x -> 2x, a, a) and a .*= 2 do the smart things, and/or provide specialized functions like scale!(a, 2), rather than encouraging the use of lower level APIs like storedvalues which again can cause problems if they aren't used properly.

[mtfishman]

@lkdvos looks good to me, I added a comment reminding us to change that line once #19 is fixed.

Can you bump the version? After that I'll merge.