Constructors

[lkdvos]

This PR changes some of the constructors of SparseArrayDOK to more closely resemble their AbstractArray counterparts, by explicitly requiring undef.
Additionally, once #32 are in place, this also ensures that inputs are checked such that unless you provide a fully-qualified type, we will check if all indices are within the correct bounds.
Also additionally, this now automatically works for any AbstractDictionary or AbstractDict, with either Cartesian or Int key types.

Finally, for convenience I also added sprand and sprand! (interface up for debate!) to facilitate easily creating random arrays for testing purposes.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 23 lines in your changes missing coverage. Please review.

Project coverage is 72.46%. Comparing base (24985a0) to head (a302199).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/sparsearraydok.jl	42.30%	15 Missing ⚠️
src/abstractsparsearray.jl	57.89%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #33      +/-   ##
==========================================
- Coverage   75.05%   72.46%   -2.60%     
==========================================
  Files           7        7              
  Lines         421      454      +33     
==========================================
+ Hits          316      329      +13     
- Misses        105      125      +20     

Flag	Coverage Δ
docs	39.46% <26.08%> (-2.64%)	⬇️

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]

Seems reasonable, I was using constructors like SparseArrayDOK{Float64}(2, 2) interchangeably as a zeros constructor and as SparseArrayDOK{Float64}(undef, 2, 2), probably best to be explicit about that. From that perspective, can we also introduce SparseArraysBase.spzeros? I can't think of better names than sprand and spzeros. Also what about SparseArraysBase.sprandn?

[mtfishman]

A question I have about sprand! is, should it zero out the input first?

[lkdvos]

I added in spzeros and followed the SparseArrays approach of having an optional rfn::Function argument in sprand(!) to allow for having a different type of random entries.
There's now an explicit call to ArrayLayouts.zero! since I don't know if we have an empty! or equivalent function in our interface?

Keep in mind that all of this is somewhat specific to non-stored entries being "zero", since otherwise these implementations become a bit ill-defined...

[mtfishman]

I added in spzeros and followed the SparseArrays approach of having an optional rfn::Function argument in sprand(!) to allow for having a different type of random entries. There's now an explicit call to ArrayLayouts.zero! since I don't know if we have an empty! or equivalent function in our interface?

I think I was using ArrayLayouts.zero! for that purpose. Not sure if there is a need to define our own SparseArraysBase.zero!, maybe that would be good and we could call out to ArrayLayouts.zero! as needed (I think overloading ArrayLayouts.zero! is important since it is part of the ArrayLayouts.jl matrix multiplication code logic, which we are using for sparse array matrix multiplication). I wouldn't want to use the name empty! since the fact that it empties the storage is an implementation detail, and it may not even do that for certain sparse array types, like DiagonalArray.

Keep in mind that all of this is somewhat specific to non-stored entries being "zero", since otherwise these implementations become a bit ill-defined...

Good point, we should start to keep track of which functions assume non-stored entries are zero and document that, and maybe check for it in functions that assume that. Maybe what we need is a trait function like SparseArraysBase.is_unstored_zero(a::AbstractArray). I've been using getunstoredindex as a way to check if the non-stored values are zero (like in the map! function) but that isn't a good idea if it is expensive to construct the zero entry, like if it is a large zero array. However, that could serve as a default definition and then sparse array types could overload SparseArraysBase.is_unstored_zero and store that information in the type/struct.

More changes requested.

[mtfishman]

@lkdvos what's the status of this PR? Merging this would make #39 easier to review (at least it would simplify the diff).

[lkdvos]

I'm definitely okay with finishing this up and merging. I think my reservations where about this being breaking (removing the SparseArrayDOK{T}(sz) constructor and only keeping the undef ones), and wanting to bundle breaking changes.
But as we discussed, versions are free so let me finish this first thing.

[mtfishman]

I'm definitely okay with finishing this up and merging. I think my reservations where about this being breaking (removing the SparseArrayDOK{T}(sz) constructor and only keeping the undef ones), and wanting to bundle breaking changes. But as we discussed, versions are free so let me finish this first thing.

Yeah, if anything it can be nicer splitting up breaking changes into separate breaking releases so then downstream packages can upgrade things incrementally.

[lkdvos]

The integration tests for BlockSparseArrays.jl are failing because of a compat issue: v0.1.0 of that package does not have compatibility bounds for the SparseArraysBase package, and thus the package manager just happily takes that one. These errors are thus unrelated.

I'll open an issue in our registry to try and get that sorted, otherwise this should be good to go.

[lkdvos]

Integration tests are now passing. This is okay to merge for me.

[mtfishman]

@lkdvos sorry to keep dragging this out, but what do you think of changing the names spzeros and sprand to sparsezeros and sparserand? My reasoning is:

1. I like longer names rather than shorthands in general.
2. We would have different names than SparseArrays.jl so it is easier to use those packages together.
3. I was thinking about how we would want to name these same kinds of functions in BlockSparseArrays.jl, and blocksparsezeros and blocksparserand looks better to me than blockspzeros and blocksprand.

[lkdvos]

No worries, I much rather change names now than in two weeks when it's used in different places as well 😉

[mtfishman]

This and #31 are good to merge/register (though I guess that one needs to be updated to reflect this one). Thanks! Great to have this kind of thing cleaned up and in a more "future proof" state.