replace ind2sub/sub2ind by CartesianIndices/LinearIndices

[jishnub]

This avoids using the internal Base methods

[codecov]

Codecov Report

Merging #451 (0711c40) into main (e86b148) will increase coverage by 0.74%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
+ Coverage   84.66%   85.41%   +0.74%     
==========================================
  Files          13       13              
  Lines        8734     8740       +6     
==========================================
+ Hits         7395     7465      +70     
+ Misses       1339     1275      -64     

Files	Coverage Δ
src/sparsematrix.jl	95.69% <100.00%> (+<0.01%)	⬆️
src/sparsevector.jl	95.43% <100.00%> (+0.01%)	⬆️

... and 2 files with indirect coverage changes

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

[ViralBShah]

Is there any perf impact?

[jishnub]

There does appear to be a small performance impact.

julia> S = sparse([1:500;], [x + (isodd(x) ? 1 : -1) for x in 1:500], [1:500;]);

julia> @btime $S[1:100_000];
  1.439 μs (2 allocations: 8.12 KiB) # master
  1.531 μs (2 allocations: 8.12 KiB) # PR

In this case, the difference appears to arise from indexing into the LinearIndices. I'm unsure why this indexing is slower, as it calls _sub2ind internally, and bounds-checking is disabled.

[ViralBShah]

I wonder what the speed impact would be on larger operations. It is a 7% slowdown in the benchmark, which may perhaps manifest as 1-2% slowdown on larger operations or applications.

[vtjnash]

I seem to get the same code for these, so maybe that was just a benchmarking artifact of some runs?

julia> LinearIndices(Matrix{Int}(undef,0,0)) |> typeof
julia> code_native((ans, Int, Int)) do x, y, z; @inbounds x[y, z]; end
julia> code_native((Tuple{Int,Int}, Int, Int)) do x, y, z; @inbounds Base._sub2ind(x, y, z); end

Seems like this should be good to merge to me