SparseMatrixCSC type check is too restrictive (regression)

[Pbellive]

A little under a month ago #31118 was merged, in order to address #31024. It introduced checks in the SparseMatrixCSC constructor to ensure that the integer type used to store the rowval and colptr arrays was wide enough. The function that performs the check is

julia/stdlib/SparseArrays/src/sparsematrix.jl

Line 36 in 54fe55b

function sparse_check_Ti(m::Integer, n::Integer, Ti::Type)

The function has the line

!isbitstype(Ti) || n*m+1 ≤ typemax(Ti) || throwTi("maximal nnz+1", "m*n+1", n*m+1)

which checks that Ti is wide enough to store the largest possible sparse matrix of size m X n, aka a dense matrix of size m X n. Shouldn't that line be

!isbitstype(Ti) || nnz+1 ≤ typemax(Ti) || throwTi("maximal nnz+1", "m*n+1", n*m+1)

where nnz = length(nzval) is the number of nonzeros entries in the matrix? Is there somewhere else in the SparseArrays stdlib where the rowval and colptr arrays need to grow for some reason? Aside from that I can't think of why the restriction above was implemented. It has broken some of my company's code where we have some quite large and very sparse matrices where we store the rowval and colptr arrays using a small integer type to save memory. The following MWE works on julia 1.1 but fails on master built from source yesterday (Commit f611b46):

julia> using SparseArrays, LinearAlgebra
julia> colptr = fill(2,101);
julia> colptr[1] = 1;
julia> rowval = [1];
A = SparseMatrixCSC(100,100,Int8.(colptr),Int8.(rowval),[2.2])
ERROR: ArgumentError: maximal nnz+1 (m*n+1 = 10001) does not fit in Ti = Int8
Stacktrace:
 [1] (::getfield(SparseArrays, Symbol("#throwTi#2")){DataType})(::String, ::String, ::Int64) at /home/patrick/Julia/master/usr/share/julia/stdlib/v1.2/SparseArrays/src/sparsematrix.jl:39
 [2] sparse_check_Ti at /home/patrick/Julia/master/usr/share/julia/stdlib/v1.2/SparseArrays/src/sparsematrix.jl:45 [inlined]
 [3] Type at /home/patrick/Julia/master/usr/share/julia/stdlib/v1.2/SparseArrays/src/sparsematrix.jl:26 [inlined]
 [4] SparseMatrixCSC(::Int64, ::Int64, ::Array{Int8,1}, ::Array{Int8,1}, ::Array{Float64,1}) at /home/patrick/Julia/master/usr/share/julia/stdlib/v1.2/SparseArrays/src/sparsematrix.jl:33
 [5] top-level scope at REPL[13]:1

Also note that the sparse_check_Ti function as written is unsafe because m and n can be any integer types and m*n will return a spurious result if m and n are of an integer type that can't hold the result of m*n.

I can submit a PR to change this unless there's some reason I'm not thinking of that the current behaviour, or at least something more restrictive than nnz+1 ≤ typemax(Ti) is required.

[mbauman]

Seem reasonable. @KlausC would you support the proposed change from n*m to nnz?

[Pbellive]

Thanks for the reply @mbauman. I've implemented the change I proposed above. I'll make a PR shortly and wait for input from @KlausC.

[ViralBShah]

Sorry I didn't see this earlier. Can't the rowval grow when you do setindex!?

[ViralBShah]

Your application does sound interesting, based on the way you are using the index types! This wasn't a use case I had imagined.

[JeffBezanson]

Fixed by #31724.