stricter buffers in SparseMatrixCSC #40523
Conversation
|
rebasing |
|
Test errors seem unrelated, rebasing. |
|
I'd say the failing check is unrelated. Could we run nanosoldier to look for performance regressions? |
|
@nanosoldier |
|
Something went wrong when running your job: Logs and partial data can be found here |
|
Is he on strike? |
|
can we uhh... retry? |
|
Looking at https://github.com/JuliaCI/NanosoldierReports/blob/master/benchmark/by_hash/421a07f_vs_d998c7e/logs/d998c7e74c84bb6f78916e71bf4efab936347170_against.out it seems it gets stuck at That's a bit suspicious since this code touches the sparse stuff. |
ah ... I'll have a look at that |
|
I tried it locally and it had no problem... Let's try again then @nanosoldier |
|
Something went wrong when running your job: Logs and partial data can be found here |
|
Ah, sorry, master is broken right now ("comparison commit") because we need this PR. @nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG |
|
If I understand correctly the only things that do not seem noise are 2 memory regressions in broadcast |
|
Yeah, LGTM |
stdlib/SuiteSparse/src/cholmod.jl
Outdated
| @@ -1970,4 +1957,58 @@ end | |||
| (*)(A::SparseVecOrMat{Float64,Ti}, | |||
| B::Hermitian{Float64,SparseMatrixCSC{Float64,Ti}}) where {Ti} = sparse(Sparse(A)*Sparse(B)) | |||
|
|
|||
| # Sort all the indices in each column for the construction of a CSC sparse matrix | |||
| # sortBuffers!(A, sortindices = :sortcols) # Sort each column with sort() | |||
| function sortBuffers!(m, n, colptr::Vector{Ti}, rowval::Vector{Ti}, nzval::Vector{Tv}) where {Ti <: Integer, Tv} | |||
There was a problem hiding this comment.
Nit, but this looks a bit non-standard to me, any special reason for the camel case?
There was a problem hiding this comment.
You're right, this comes from sortSparseMatrixCSC that was in sparsematrix.jl (which I didn't even remove for the moment), I'll rename it (it should not be exported though).
vtjnash
added
status:triage
|
I think this can be merged, once the reviewer comments are addressed. Any objections? |
|
the memory regression should not be there, I'll investigate... |
* Add sizehint!(::SparseMatrixCSC, args...), * Fix illegal SparseMatrixCSC construction in cholmod and linalg. * Remove tests targetting now illegal buffers * Fix invalid buffer creation in kron and more
|
Should be fixed. The problem was that by removing the |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG |
vtjnash
changed the title
vtjnash
added
the
status:merge me
|
The memory stuff seems gone. The other performance stuff that appeared seems funky... |
dkarrasch
removed
the
status:merge me
Make length(A.nzval)==nnz(A) and add strict buffer checking (#30662) * Add sizehint!(::SparseMatrixCSC, args...), * Fix illegal SparseMatrixCSC construction in cholmod and linalg. * Remove tests targetting now illegal buffers * Fix invalid buffer creation in kron and more * use widelength in sizehint! to cope with large matrices in 32 bit systems
Make length(A.nzval)==nnz(A) and add strict buffer checking (#30662) * Add sizehint!(::SparseMatrixCSC, args...), * Fix illegal SparseMatrixCSC construction in cholmod and linalg. * Remove tests targetting now illegal buffers * Fix invalid buffer creation in kron and more * use widelength in sizehint! to cope with large matrices in 32 bit systems
Make length(A.nzval)==nnz(A) and add strict buffer checking (#30662) * Add sizehint!(::SparseMatrixCSC, args...), * Fix illegal SparseMatrixCSC construction in cholmod and linalg. * Remove tests targetting now illegal buffers * Fix invalid buffer creation in kron and more * use widelength in sizehint! to cope with large matrices in 32 bit systems
Make length(A.nzval)==nnz(A) and add strict buffer checking (#30662) * Add sizehint!(::SparseMatrixCSC, args...), * Fix illegal SparseMatrixCSC construction in cholmod and linalg. * Remove tests targetting now illegal buffers * Fix invalid buffer creation in kron and more * use widelength in sizehint! to cope with large matrices in 32 bit systems
With this patch the output buffers to `sparse!` are resized in order to satisfy the buffer length checks in the `SparseMatrixCSC` constructor that were introduced in JuliaLang/julia#40523. Previously `csccolptr` was never resized, and `cscrowval` and `cscnzval` were only resized if the buffers were too short (i.e. never truncated). The requirement `length(csccolptr) >= n + 1` could be kept, but seems unnecessary since all buffers need to be resized anyway (to pass the constructor checks). In particular this fixes calling `sparse!` with `I`, `J`, `V` as both input and output buffers: `sparse!(I, J, V, m, n, ..., I, J, V)`. Fixes #313.
With this patch the output buffers to `sparse!` are resized in order to satisfy the buffer length checks in the `SparseMatrixCSC` constructor that were introduced in JuliaLang/julia#40523. Previously `csccolptr` was never resized, and `cscrowval` and `cscnzval` were only resized if the buffers were too short (i.e. never truncated). The requirement `length(csccolptr) >= n + 1` could be kept, but seems unnecessary since all buffers need to be resized anyway (to pass the constructor checks). In particular this fixes calling `sparse!` with `I`, `J`, `V` as both input and output buffers: `sparse!(I, J, V, m, n, ..., I, J, V)`. Fixes #313.
…314) With this patch the output buffers to `sparse!` are resized in order to satisfy the buffer length checks in the `SparseMatrixCSC` constructor that were introduced in JuliaLang/julia#40523. Previously `csccolptr` was never resized, and `cscrowval` and `cscnzval` were only resized if the buffers were too short (i.e. never truncated). The requirement `length(csccolptr) >= n + 1` could be kept, but seems unnecessary since all buffers need to be resized anyway (to pass the constructor checks). In particular this fixes calling `sparse!` with `I`, `J`, `V` as both input and output buffers: `sparse!(I, J, V, m, n, ..., I, J, V)`. Fixes #313.
…314) With this patch the output buffers to `sparse!` are resized in order to satisfy the buffer length checks in the `SparseMatrixCSC` constructor that were introduced in JuliaLang/julia#40523. Previously `csccolptr` was never resized, and `cscrowval` and `cscnzval` were only resized if the buffers were too short (i.e. never truncated). The requirement `length(csccolptr) >= n + 1` could be kept, but seems unnecessary since all buffers need to be resized anyway (to pass the constructor checks). In particular this fixes calling `sparse!` with `I`, `J`, `V` as both input and output buffers: `sparse!(I, J, V, m, n, ..., I, J, V)`. Fixes #313. (cherry picked from commit 85a381b)
…314) With this patch the output buffers to `sparse!` are resized in order to satisfy the buffer length checks in the `SparseMatrixCSC` constructor that were introduced in JuliaLang/julia#40523. Previously `csccolptr` was never resized, and `cscrowval` and `cscnzval` were only resized if the buffers were too short (i.e. never truncated). The requirement `length(csccolptr) >= n + 1` could be kept, but seems unnecessary since all buffers need to be resized anyway (to pass the constructor checks). In particular this fixes calling `sparse!` with `I`, `J`, `V` as both input and output buffers: `sparse!(I, J, V, m, n, ..., I, J, V)`. Fixes #313. (cherry picked from commit 85a381b)
This is a rebase of #30676, without the addition of a capacity-querying function.
The general idea is about making
length(nonzeros(A))==nnz(A)inSparseMatrixCSC, with strict buffer checking on exported functions (a few internal functions still return illegal buffers because the construction is done in several places, in that cases we create an empty matrix and resize the buffers afterwards. They should be clearly marked by comments)Adresses #30662. See also: #26560, #30435.
Still WIP.