-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Labels
Comments
Seem reasonable. @KlausC would you support the proposed change from |
ararslan
added
the
regression
Regression in behavior compared to a previous version
label
Apr 9, 2019
Sorry I didn't see this earlier. Can't the |
Your application does sound interesting, based on the way you are using the index types! This wasn't a use case I had imagined. |
ViralBShah
pushed a commit
that referenced
this issue
Jun 26, 2019
* reconstruct PR #31118 * reconstruct PR 31118 2 * Check arguments of SparseMatrixCSC #31024 #31435 * fix SuiteSparse test * added NEWS, fixed tests * loosen restrictions - resize to useful length * cleaned up NEWS, revert minor change * add non-checking and checking constructor - improve check performance
Fixed by #31724. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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 therowval
andcolptr
arrays was wide enough. The function that performs the check isjulia/stdlib/SparseArrays/src/sparsematrix.jl
Line 36 in 54fe55b
The function has the line
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 bewhere
nnz = length(nzval)
is the number of nonzeros entries in the matrix? Is there somewhere else in theSparseArrays
stdlib where therowval
andcolptr
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 therowval
andcolptr
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):Also note that the
sparse_check_Ti
function as written is unsafe because m and n can be any integer types andm*n
will return a spurious result if m and n are of an integer type that can't hold the result ofm*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.The text was updated successfully, but these errors were encountered: