Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SparseMatrixCSC type check is too restrictive (regression) #31435

Open
Pbellive opened this issue Mar 21, 2019 · 4 comments

Comments

4 participants
@Pbellive
Copy link
Contributor

commented Mar 21, 2019

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

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

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

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

@mbauman mbauman added the sparse label Apr 9, 2019

@Pbellive

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

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

@ViralBShah

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

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

KlausC added a commit to KlausC/julia that referenced this issue Apr 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.