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

Argument checks for SparseMatrixCSC constructors #31724

Merged
merged 11 commits into from
Jun 26, 2019
Merged

Argument checks for SparseMatrixCSC constructors #31724

merged 11 commits into from
Jun 26, 2019

Conversation

KlausC
Copy link
Contributor

@KlausC KlausC commented Apr 14, 2019

This PR combines the reverted breaking #31118 and the unfinished #31661.

In order to allow n * m >= typemax(Ti) it is necessary to put the following restrictions in place is order to avoid segmentation violations.

  1. m, n <= typemax(Ti)
  2. length(colptr) >= n+1
  3. 1 <= colptr[i] <= colptr[i+1] for i in 1:n
    in addition to the preexisting checks.
    All uses of SparseMatrixCSC(m, n, colptr, ....) with uninitialized colptr have been modified.

Fixes #31024.

@ViralBShah ViralBShah requested a review from mbauman April 14, 2019 21:43
@ViralBShah ViralBShah added the sparse Sparse arrays label Apr 14, 2019
@Pbellive
Copy link
Contributor

Thanks for getting this done @KlausC! This solution looks good to me.

stdlib/SparseArrays/src/sparsematrix.jl Outdated Show resolved Hide resolved
@KlausC
Copy link
Contributor Author

KlausC commented Apr 15, 2019

I wanted to add the following text into NEWS.md:

* `SparseMatrixCSC(m,n,colptr,rowval,nzval)` and `sparse(I, J, V)` perform consistency checks
  for some arguments. `colptr` must be properly populated and lengths of `colptr`, `rowval`,
  and `nzval` must be compatible with `m`, `n`, and `eltype(colptr)`.

Also: # add tests for length(I) > typemax(Ti) when n*m < typemax(Ti)

@mbauman
Copy link
Member

mbauman commented Apr 15, 2019

Go for it! You can just tag the PR as WIP in the title to prevent someone from prematurely merging it before you're done.

@KlausC KlausC changed the title Argument checks for SparseMatrixCSC constructors WIP - Argument checks for SparseMatrixCSC constructors Apr 15, 2019
@KlausC KlausC changed the title WIP - Argument checks for SparseMatrixCSC constructors Argument checks for SparseMatrixCSC constructors Apr 15, 2019
@KlausC
Copy link
Contributor Author

KlausC commented Apr 25, 2019

bumpi

@KlausC KlausC closed this Apr 25, 2019
@KlausC KlausC reopened this Apr 25, 2019
@StefanKarpinski
Copy link
Member

Both failures seem unrelated (Win32: FileWatching; macOS: can't reach github.com).

@StefanKarpinski
Copy link
Member

@mbauman or @KristofferC, please merge if this looks good to you.

@mbauman
Copy link
Member

mbauman commented Apr 25, 2019

Edit: whoops, wrong comment on the wrong PR. Sorry about that.

@KlausC
Copy link
Contributor Author

KlausC commented May 2, 2019

bump often!

@KlausC
Copy link
Contributor Author

KlausC commented May 14, 2019

bump :-)

@jebej
Copy link
Contributor

jebej commented May 14, 2019

How much slower does this make the matrix construction?

@jebej
Copy link
Contributor

jebej commented May 14, 2019

In one particular use case for me, this makes the matrix construction 7x slower. If we want to add this, please make it an outer constructor. Unless I missed something about the original issue, I don't think this is desirable. People who use the direct constructor should know what they are doing.

@jebej
Copy link
Contributor

jebej commented May 14, 2019

If I understand properly, it seems that the real issue originally is due to the sparse constructor giving invalid arguments to SparseMatrixCSC. If that's the case, could we fix the issue there instead and leave SparseMatrixCSC alone?

@KlausC
Copy link
Contributor Author

KlausC commented May 23, 2019

@jebej, I try to understand your argument. Actually I do not agree with

People who use the direct constructor should know what they are doing

I think, everybody in all situations should know what he is doing, but the 'should' is only a weak warranty. There is also no hint in the documentation like 'if you use this constructor, you should guarantee the consistency of all arguments, otherwise a segfault might occur'.

I am in favor of input argument checks, if they do not add inappropriate effort.
The checks added in this PR add approximately n integer comparisons, where n is the number of columns of the matrix. The initial setting of the vector colptr with valid entries requires typically n write operations, which could be at least as expensive. Taking this in account, I would expect in the worst case only doubling of the construction time. I would appreciate to see your '7x' use case.

Of course it would be possible to move the check from the constructor to sparse and all other uses of the constructor within the standard libraries. I think, it is a design decision.

@KlausC
Copy link
Contributor Author

KlausC commented May 29, 2019

bump :-)

@ViralBShah
Copy link
Member

@jebej Any thoughts here?

@jebej
Copy link
Contributor

jebej commented Jun 17, 2019

I think what I had said earlier still applies, the original issue is with the sparse function giving bad inputs to the constructor SparseMatrixCSC, and is not with the constructor itself.

I would consider the SparseMatrixCSC constructor an "experts-only" constructor, given that you need to understand how to set the column pointers properly. It would be sad to have that constructor make all these extraneous (again, for "experts") checks without a way to turn them off. If we still want to add checks to this constructor, there should be a way to bypass them.

Regarding the test I made, it was the creation of an annihilation operator:

function destroy_checked(::Type{T}, N::Integer) where {T<:Number}
    rowval = Vector{Int}(undef,N-1); for i=1:N-1; @inbounds rowval[i]=i; end
    colptr = Vector{Int}(undef,N+1); colptr[1]=1; for i=2:N+1; @inbounds colptr[i]=i-1; end
    nzval  = Vector{T}(undef,N-1); for i=1:N-1; @inbounds nzval[i]=√(T(i)); end
    return SparseMatrixCSC_checked(N,N,colptr,rowval,nzval)
end

which gives, for the checked and unchecked cases:

julia> @benchmark destroy_checked(Float64,10)
BenchmarkTools.Trial:
  memory estimate:  560 bytes
  allocs estimate:  5
  --------------
  minimum time:     791.657 ns (0.00% GC)
  median time:      854.485 ns (0.00% GC)
  mean time:        988.225 ns (8.41% GC)
  maximum time:     490.635 μs (99.70% GC)
  --------------
  samples:          10000
  evals/sample:     99

julia> @benchmark destroy(Float64,10)
BenchmarkTools.Trial:
  memory estimate:  544 bytes
  allocs estimate:  4
  --------------
  minimum time:     148.063 ns (0.00% GC)
  median time:      165.921 ns (0.00% GC)
  mean time:        199.120 ns (12.07% GC)
  maximum time:     59.564 μs (99.27% GC)
  --------------
  samples:          10000
  evals/sample:     836

So more like 5x.

@jebej
Copy link
Contributor

jebej commented Jun 17, 2019

I would of course support adding a blurb to the documentation saying that the constructor does not check inputs, and to use sparse if input checking is needed.

@KlausC
Copy link
Contributor Author

KlausC commented Jun 18, 2019

I improved the PR in to give attention to the above objections:

  1. Performance of checks was improved. The time penalty is now 15 + 0.7 * N Nanoseconds, where N is the array size in the example.

  2. The constructor SparseMatrixCSC{Tv,Ti}(m, n, ...) is the unchecked "expert" version now. It may be used, if checks are not wanted. The constructor SparseMatrixCSC(m, n, ...) has the additional features.

@KlausC
Copy link
Contributor Author

KlausC commented Jun 18, 2019

Benchmarks:

julia> function destroy(::Type{T}, ::Type{Ti}, N::Integer, check=true) where {T<:Number,Ti<:Integer}
           rowval = Vector{Ti}(undef,N-1); for i=1:N-1; @inbounds rowval[i]=i; end
           colptr = Vector{Ti}(undef,N+1); colptr[1]=1; for i=2:N+1; @inbounds colptr[i]=i-1; end
           nzval  = Vector{T}(undef,N-1); for i=1:N-1; @inbounds nzval[i]=√(T(i)); end
           check ? SparseMatrixCSC(N,N,colptr,rowval,nzval) : SparseMatrixCSC{T,Ti}(N,N,colptr,rowval,nzval)
       end
julia> function benches(::Type{Ti}) where Ti
           bf = []; bt = []
           for n in 0:6
               push!(bf, @benchmark destroy(Float64, $Ti, $(10^n), false))
               push!(bt, @benchmark destroy(Float64, $Ti, $(10^n), true))
           end
           bf, bt
       end
benches (generic function with 1 method)

julia> bf, bt = benches(Int);             

julia> bt[2]
BenchmarkTools.Trial: 
  memory estimate:  560 bytes
  allocs estimate:  5
  --------------
  minimum time:     151.764 ns (0.00% GC)
  median time:      159.621 ns (0.00% GC)
  mean time:        252.140 ns (34.91% GC)
  maximum time:     4.746 μs (95.96% GC)
  --------------
  samples:          10000
  evals/sample:     832

julia> bf[2]
BenchmarkTools.Trial: 
  memory estimate:  544 bytes
  allocs estimate:  4
  --------------
  minimum time:     128.658 ns (0.00% GC)
  median time:      137.627 ns (0.00% GC)
  mean time:        223.338 ns (36.60% GC)
  maximum time:     4.196 μs (96.33% GC)
  --------------
  samples:          10000
  evals/sample:     898


julia> times(f, b) = getfield.(f.(b), :time)
times (generic function with 1 method)

julia> timediff(f, bf, bt) = times(f, bt) .- times(f, bf)
timediff (generic function with 1 method)

julia> timequot(f, bf, bt) = times(f, bf) ./ times(f, bt)
timequot (generic function with 1 method)

# median time differences in Nanoseconds:
julia> b = timediff(median, bf, bt)
7-element Array{Float64,1}:
     14.459207056651152
     21.993844494175107
     75.44275081162402 
    654.5              
   6104.5              
  61135.0              
 640836.0              

julia> A = [ones(7) [1,10,100,1000,10^4,10^5,10^6]]
julia> D = inv(diagm([1,10,100,1000,10^4,10^5,10^6]));

# linear fitting with weigths:
julia> (A'D*A) \ (A'D*b)
2-element Array{Float64,1}:
 13.918559381172859 
  0.6378700830850331
# the time values of b are regenerated with good accuracy:
julia> A * ((A'D*A) \ (A'D*b))
7-element Array{Float64,1}:
     14.556429464257892
     20.29726021202319 
     77.70556768967617 
    651.7886424662059  
   6392.6193902315035  
  63800.92686788448    
 637884.0016444143 

# time increase is between 7 and 14 %: 
julia> timequot(median, bt, bf)
7-element Array{Float64,1}:
 1.1403098489801728
 1.159807688028621 
 1.1149221696978133
 1.0931805239179955
 1.0946441445282522
 1.095640148368556 
 1.0756124851287872

@KlausC
Copy link
Contributor Author

KlausC commented Jun 19, 2019

@jebej, do you still see room for improvements?

@KristofferC
Copy link
Member

Personally, I don't think these things should be checked at construction time since there are cases where you intermediately might want to have an invalid sparse matrix.
It also seems odd to me to have SparseMatrixCSC{} do no checking but SparseMatrixCSC do checking. These feel like they should do the same thing with the exception that in the first case you get the types you explicitly specify. In addition, the SparseMatrixCSC(...) construtor is from what I can see not even documented (https://docs.julialang.org/en/v1/stdlib/SparseArrays/index.html#Sparse-Vector-and-Matrix-Constructors-1) so adding too much logic to it seems not very useful.

I would personally prefer something like #22529 (which is similar to https://docs.scipy.org/doc/scipy-1.2.0/reference/generated/scipy.sparse.csr_matrix.check_format.html#scipy.sparse.csr_matrix.check_format) where you can do a check to see that the matrix is valid before you start to use it. This also allows you to check that the matrix is valid after performing mutating functions on the fields, and not only at construction time.

@KlausC
Copy link
Contributor Author

KlausC commented Jun 19, 2019

Personally, I don't think these things should be checked at construction time since there are cases where you intermediately might want to have an invalid sparse matrix.

That is contrary to my opinion: I think, every exported constructor, if documented or not, should return a valid object of that type.
There should be only few exceptions to this rule, and the consequences of leaking an invalid object to general usage should be (invalid state) exceptions, but neither seg-faults nor silent failure.
In the case of SparseMatrixCSC there are @inbounds statements and silent assumptions about the correctness of the data at many places in the SparseArrays package. Application of those functions to invalid matrices would in the worst case silently produce wrong results.

since there are cases where you intermediately might want to have an invalid sparse matrix

in such cases it is easy to postpone construction until integrity of components has been established. At least in the julia base and stdlib that could be easily achieved.

seems odd to me to have SparseMatrixCSC{} do no checking but SparseMatrixCSC do checking

That was just a proposal to provide an unchecked version of the constructor besides the checked version, as was suggested by @jebej.

not even documented

If it is exported it should be documented.

... so adding too much logic to it seems not very useful

The logic added to the constructor is:

  1. check m, n <= typemax(Ti). (even better the field types of m, n would be Ti). That is not more logic added than the current m,n > 0.
  2. check of monotony of colptr is more effort, but not much compared to building the component arrays
  3. resize vectors to maximal potential valid required lengths is just reasonable and typically a nop
    I don't think that is much added logic.

also allows you to check that the matrix is valid after performing mutating functions on the fields, and not only at construction time

Adding a (exported?) function to perform a validity check is a good idea and should share code with the checks of this PR.

@KristofferC
Copy link
Member

If it is exported it should be documented.

The type SparseMatrixCSC is documented. The constructor SparseMatrixCSC(...) is not.

@KlausC
Copy link
Contributor Author

KlausC commented Jun 19, 2019

That looks like missing documentation, not like "don't use the constructor". What I mean is, if type SparseMatrixCSC exported and its constructors SparseMatrixCSC(....) are not supported/fragile/not-to-be-used by "non-experts", there should be a hint to that, because it is unexpected.

@KlausC
Copy link
Contributor Author

KlausC commented Jun 19, 2019

To be constructive, I want to outline a way to save the essence of this PR without touching the constructors.

  1. revert all constructors to the previous state.
  2. collect all additional checks of this PR into a separate function checkvalid, which also integrates all implement a checkvalid function for sparse matrices and use it in show #22529 checks.
  3. create a new method sparse(m, n, colptr, rowval, nzval) which is documented as an alternative way of creating a valid SparseMatrixCSC and with the features of this commit d545e34
  4. use validation within SparseArrays in in addition to the constructor, where checking seems useful; that includes sparse(I, J, V, ...).

@ViralBShah
Copy link
Member

The challenge here comes from the fact that SparseMatrixCSC was not originally designed to be user facing. It was imagined that if anyone used it, they would need to fully take ownership of getting it right. I think it would be fine to implement checks 1 and 2 in the constructor since those are cheap and won't hurt anything. The colptr checks are best not done, because people expect the constructors to be cheap (historically) and this would surprise them.

I am not convinced about extending the sparse method signature to add the low-level matrix constructor. For now, let's do the checkvalid thing and get this one merged. We can plan other convenient ways of constructing validated CSC structures separately. Could even be in a package.

I feel that this whole thing needs to move out of stdlib so we can iterate faster and do more, but that needs stdlib versioning and a bunch more stuff.

@KlausC
Copy link
Contributor Author

KlausC commented Jun 21, 2019

I adapted my outline according to your proposals.

Copy link
Member

@ViralBShah ViralBShah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the PR looks good to me. So hopefully we can merge this soon.

@ViralBShah
Copy link
Member

ViralBShah commented Jun 26, 2019

I am willing to give this PR a shot, with the SparseMatrixCSC{Tv,Ti} as the expert version and SparseMatrixCSC checking the colptr and a few more things.

Just a note - the checking of the colptr especially in SparseMatrixCSC is an issue after someone has written all over it - so you want to check it at that point before it is handed over to other code.


Tj = Ti
while isbitstype(Tj) && coolen >= typemax(Tj)
Tj = widen(Tj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made Tj type unstable and caused the regression in #32985. Only matters for smallish arrays but would be good to fix nonetheless. Can't we just always use Int for this or something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would just be an error. This segfaulted in 1.2. Let's just make it an error instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KristofferC
Copy link
Member

The error messages here leave some to be desired. For example

julia> SparseMatrixCSC(5, 1, Int8[1,2], fill(Int8(1),127), Int[1,2,3])
ERROR: ArgumentError: 127 == length(rowval) >= 127

gives very little information about what went wrong and how it can be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bound violation in SparseArrays - segfault
7 participants