Refactoring: Use accessor methods when manipulating sparse matrices/vectors

[tkf]

This is the first step towards extensible sparse arrays. Based on discussion with @ViralBShah #30173 (comment), I changed all code touching fields of SparseMatrixCSC and SparseVector with the corresponding accessor methods that already exist.

I'd like to make this PR minimal and mechanistic to focus on the refactoring side. I prefer to do the second step mentioned in #30173 (comment) later.

[ViralBShah]

Thanks. This is fantastic! Let me know when good to merge. Can you do a quick test to ensure no performance regressions have crept in?

[tkf]

I did all I wanted to do this in PR. Please merge it whenever you think it's ready.

I just run a few quick benchmarks. I don't find any sign of regressions in this branch. My impression from running the same benchmark a few times by hand is that the difference is likely within the trial-to-trial fluctuations.

This branch:

julia> @btime mul!($(zeros(100, 100)), $(sprandn(100, 100, 0.1)), $(randn(100, 100)));
  79.478 μs (0 allocations: 0 bytes)

julia> @btime mul!($(zeros(100, 100)), $(sprandn(100, 100, 0.1)'), $(randn(100, 100)));
  73.109 μs (0 allocations: 0 bytes)

julia> @btime $(zeros(100, 100)) .= $(sprandn(100, 100, 0.1)) .* $(randn(100, 100));
  56.604 μs (0 allocations: 0 bytes)

With master b0ff444:

julia> @btime mul!($(zeros(100, 100)), $(sprandn(100, 100, 0.1)), $(randn(100, 100)));
  83.647 μs (0 allocations: 0 bytes)

julia> @btime mul!($(zeros(100, 100)), $(sprandn(100, 100, 0.1)'), $(randn(100, 100)));
  69.138 μs (0 allocations: 0 bytes)

julia> @btime $(zeros(100, 100)) .= $(sprandn(100, 100, 0.1)) .* $(randn(100, 100));
  58.008 μs (0 allocations: 0 bytes)

julia> VERSION
v"1.3.0-alpha.154"

[ViralBShah]

Is it possible to do some Compat stuff to prevent breakage? I think there is rampant use of sparse matrix fields everywhere. Do we want to make this a deprecation in 1.3 and remove direct field access in only 1.4?

Technically this wasn't a documented API, so it is not strictly necessary, but it might be nice to have. Also, we should make these accessor methods documented APIs. Should also go into NEWS. Can do in subsequent PRs.

[ViralBShah]

Actually, I wonder whether we should drop get from getcolptr for consistency. The next step would be to harmonize the APIs across SparseMatrixCSC and SparseVector to the extent possible.

[tkf]

Thanks for the very quick review and merge!

Is it possible to do some Compat stuff to prevent breakage? I think there is rampant use of sparse matrix fields everywhere.

I tried to not break anything. Field access is still allowed:

julia> A = spzeros(10, 10)
10×10 SparseMatrixCSC{Float64,Int64} with 0 stored entries

julia> A.nzval
0-element Array{Float64,1}

If you are referring to Base.getproperty(S::SparseMatrixCSC, ::Symbol) = error("use accessor function") etc. I mentioned in #32953 (comment), this definition is applied only in the test suite so there is no effect in the normal usecase (unless you do include("stdlib/LinearAlgebra/test/runtests.jl") in your REPL).

Actually, I wonder whether we should drop get from getcolptr for consistency.

Yes, I think that makes sense.

Relate to this, I think it's possible to get rid of the majority of getcolptr usage by nzrange (see #33037). I think it results in more readable code.

[ViralBShah]

Ah - I missed that the getproperty was only for tests. What do you think about making it a deprecation warning - in order to move everyone to accessor methods?

[tkf]

It looks like the accessor like nonzeros have exited for more than 4 years since Julia 0.4 (#8720) so I'd prefer to assume that all existing and maintained code base already have migrated to the accessor methods. But maybe it's better to open an issue so that people can be aware of this discussion?

[tkf]

Can you remove needs docs and needs news tags? I guess we can do this in the PR that actually deprecates field access?

[ViralBShah]

Maybe we should open up an issue for the deprecation of direct field access before making a PR. It may not be a popular decision.

We should still document the accessor API and put the accessors as the preferred way for the NEWS - even if we don't end up deprecating direct field access.

[tkf]

We should still document the accessor API and put the accessors as the preferred way for the NEWS - even if we don't end up deprecating direct field access.

Got it. Please see #33050.