implement a checkvalid function for sparse matrices and use it in show #22529

KristofferC · 2017-06-25T05:11:24Z

tkelman · 2017-06-25T07:17:09Z

base/sparse/sparsematrix.jl

+```
+"""
+function Base.isvalid(S::SparseMatrixCSC; full = false)
+


no blank line at start of a block

tkelman · 2017-06-25T07:18:28Z

base/sparse/sparsematrix.jl

+
+    if full
+        # Check colptr increasing
+        if length(S.colptr) > 1 && !all(i -> S.colptr[i+1] >= S.colptr[i], 1:length(S.colptr)-1)


would issorted be any faster?

tkelman · 2017-06-25T07:20:49Z

base/sparse/sparsematrix.jl

+    return true
+end
+
+_print_invalid(io, S) = print(io, typeof(S), " with invalid internal representation")


still worth showing the size

tkelman · 2017-06-25T07:21:35Z

This should verify that the row indices are strictly sorted within each column, possibly under a different kwarg

nalimilan · 2017-06-25T08:49:28Z

Thanks! However to be completely sure we can close #14489 after merging this, we would need to be check that no segfault can happen in other functions for invalid matrices (or at least, that the impact of fixing these crashes on performance would be acceptable).

KristofferC · 2017-06-25T15:13:01Z

Added a verification for rowindices in a column
Added size when printing invalid matrices
Removed closing statement of issue from first post.

tkelman · 2017-06-25T15:29:42Z

base/sparse/sparsematrix.jl

+false
+```
+"""
+function Base.isvalid(S::SparseMatrixCSC; full = false)


why defaulting to only partial checks?

Yeah, perhaps full should be default. I'll change it.

tkelman · 2017-06-25T15:30:26Z

base/sparse/sparsematrix.jl

+        # Check rowval in range and sorted in each column
+        for col in 1:S.n
+            nzrang = nzrange(S, col)
+            l_nzrange = length(nzrang)


unused variable

tkelman · 2017-06-25T15:30:45Z

base/sparse/sparsematrix.jl

+                ri = S.rowval[r]
+                (ri <= 0 || ri > S.m) && return false
+                r == last(nzrang) && break
+                S.rowval[r+1] <= ri && (println("returning"); return false)


debugging print left in?

tkelman · 2017-06-25T15:32:55Z

test/sparse/sparse.jl

+
+@testset "isvalid" begin
+    for x in [sprand(10,5,0.5), sprand(5,10,0.5), spzeros(0,0), spzeros(2,2)]
+        @show x


debugging print left in

KristofferC · 2017-06-25T15:33:38Z

base/sparse/sparsematrix.jl

+                ri = S.rowval[r]
+                (ri <= 0 || ri > S.m) && return false
+                r == last(nzrang) && break
+                S.rowval[r+1] <= ri && (println("returning"); return false)


KristofferC · 2017-06-25T15:58:26Z

Changed full to true by default
Some other minor cleanups

Sacha0 · 2017-06-26T21:58:31Z

base/sparse/sparsematrix.jl

+
+Check if a sparse matrix `S` has a valid internal representation.
+If `full` is `false` only O(1) checks are done, otherwise full
+O(nnz) checks are performed.


Should O(1) and O(nnz) be marked up somehow?

Sacha0 · 2017-06-26T22:04:02Z

base/sparse/sparsematrix.jl

+```
+"""
+function Base.isvalid(S::SparseMatrixCSC; full = true)
+    length(S.colptr) != S.n + 1 && return false


This check seems too strict. For explanation, please see #20464 (comment) :).

Sacha0

Thanks @KristofferC! :)

Re. length checks, I advocate checking the set of constraints described in/around #20464 (comment) and no more. Thoughts? :)

KristofferC · 2017-06-27T03:35:06Z

Thanks for pointing me to #20464. Will update accordingly.

KristofferC · 2017-06-27T06:23:07Z

Updated. I mimicked a bit the checkbounds style where the code can either throw an exception or return true / false if a Type{Bool} is given as first argument.

tkelman · 2017-06-27T06:24:06Z

base/sparse/sparsematrix.jl

+```
+"""
+function checkvalid(::Type{Bool}, S::SparseMatrixCSC; full = true)
+    return _checkvalid(S; full = full) == SparseArrayInvalid.VALID


Invalid.VALID is a messy name here

Okay. Please suggest better name / names. SparseArrayValidity?

tkelman · 2017-06-27T06:24:30Z

base/sparse/sparsematrix.jl

+Throw an [`ArgumentError`](@ref) if the sparse matrix `S` does not have a valid internal representation.
+If `full` is `false` only O(1) checks are done, otherwise full
+O(nnz) checks are performed.
+```


no code here...

Please elaborate.

Oh, I see now.

KristofferC · 2017-06-27T06:30:12Z

Updated name of module encapsulating enum as well as removed unwanted code block from a docstring.

Sacha0 · 2017-06-27T17:29:41Z

base/sparse/sparsematrix.jl

+
+
+"""
+    checkvalid(Bool, S::SparseMatrixCSC; full = true) -> Bool


Perhaps Bool should be ::Type{Bool}? I was not certain whether the first argument should be a boolean value or the Bool type prior to reading the examples. Or do you prefer this signature with the examples to clarify?

Hmm, looking at the docs for checkbounds:

checkbounds(Bool, A, I...)

Return true if the specified indices I are in bounds for the given array A.
Subtypes of AbstractArray should specialize this method if they need to
provide custom bounds checking behaviors; however, in many cases one can
rely on A's indices and checkindex.

so this is consistent with that. In my opinion ::Type{Bool} is more clear though. Thoughts?

Whichever you deem best :).

this isn't right as a signature, it would locally overwrite Bool as a variable name

Pushed a commit that changes this for the pertinent check-methods.

Sacha0 · 2017-06-27T17:32:54Z

base/sparse/sparsematrix.jl

+        err_str = "S.rowval must be sorted for all column ranges"
+    else # Should not happen
+        err_str = "sparse array is invalid"
+    end


Beautiful error messages! :) Perhaps the code snippets should be quoted?

Good point. Will update accordingly.

Regarding quoting error messages; do we usually quote things in error message strings? They will not be rendered as markdown and I'm not sure if we use "`" as a convention for code in strings.

we frequently do, yes

I was checking e.g. https://github.com/JuliaLang/julia/pull/20464/files#diff-fceaad3480ef8dc3a224be725fa10678R866. Anyway, updated with code quotes.

Sacha0

Beautiful! :) 💯

…into kc/sparse_check

tkelman · 2017-06-28T05:48:51Z

base/sparse/sparsematrix.jl

@@ -167,19 +167,19 @@ function checkvalid(S::SparseMatrixCSC; full = true)
    validity = _checkvalid(S; full = full)
    validity == SparseArrayValidity.VALID && return
    if validity == SparseArrayValidity.COLPTR_LENGTH
-        err_str = "length of S.colptr must be at least size(S,2) + 1 = $(S.n + 1) but was $(length(S.colptr))"
+        err_str = "length of `S.colptr` must be at least `size(S,2)` + 1 = $(S.n + 1) but was $(length(S.colptr))"


would include the + 1 or - 1 in the highlighting

As you wish.

Sacha0 · 2017-06-28T17:56:44Z

base/sparse/sparsematrix.jl

+O(nnz) checks are performed.
+
+# Examples
+


(Extra empty line under the seemingly just-established convention, but could be fixed in the dedicated PR instead.)

Sacha0

lgtm for the touchups! :)

tkelman · 2017-06-28T18:04:31Z

base/sparse/sparsematrix.jl

+    elseif validity == SparseArrayValidity.COLPTR_SORTED
+        err_str = "`S.colptr` must be sorted"
+    elseif validity == SparseArrayValidity.ROWVAL_RANGE
+        err_str = "each element in `S.rowval` must be > 0 and ⩽ `size(S,1)`"


should use ≤, not \leqslant

KristofferC · 2017-08-05T14:35:05Z

Comments addressed

tkelman · 2017-08-05T14:43:56Z

test/sparse/sparse.jl

+            ]
+        @test SparseArrays._checkvalid(mat; full=full) == err
+        @test SparseArrays.checkvalid(Bool, mat; full=full) == false
+        @test_throws ArgumentError SparseArrays.checkvalid(mat; full=full)


would it be worth adding

if full @test SparseArrays._checkvalid(mat; full=false) == SparseArrays.SparseArrayValidity.VALID @test SparseArrays.checkvalid(Bool, mat; full=false) == true @test SparseArrays.checkvalid(mat; full=false) === nothing end

for coverage of that case?

ViralBShah · 2018-12-17T03:38:45Z

It would be nice to revive this one.

ViralBShah · 2019-06-26T22:05:05Z

@KlausC implemented much of this in b32c1b8. I think all we need is a checkvalid wrapper.

ViralBShah · 2020-03-04T03:44:07Z

@KristofferC Is it ok to close?

KristofferC · 2021-04-07T08:18:50Z

I think this is still useful (ref https://discourse.julialang.org/t/weird-duplicated-entries-with-different-values-in-sparsematrixcsc/).

ViralBShah · 2021-04-07T20:45:58Z

Definitely useful if we can rebase and merge.

DilumAluthge · 2022-01-14T22:48:40Z

It looks like this PR touches three files. One of those files (base/abstractarray.jl) is in Base. The remaining two files are part of the SparseArrays stdlib.

We have moved the SparseArrays stdlib to an external repository. Therefore, for the portion of this PR that modifies the SparseArrays stdlib, can you please:

Remove the SparseArrays changes from this PR.
Open a new PR against https://github.com/JuliaLang/SparseArrays.jl with the SparseArrays changes.

Thank you!

ViralBShah · 2022-01-17T19:46:42Z

SparseArrays.jl is now in its own separate repo. Please open the PR there if still relevant.

KristofferC added the sparse Sparse arrays label Jun 25, 2017

tkelman reviewed Jun 25, 2017

View reviewed changes

KristofferC force-pushed the kc/sparse_check branch 2 times, most recently from 8319166 to d4ccc4e Compare June 25, 2017 15:12

tkelman reviewed Jun 25, 2017

View reviewed changes

KristofferC commented Jun 25, 2017

View reviewed changes

KristofferC force-pushed the kc/sparse_check branch from d4ccc4e to 155e288 Compare June 25, 2017 15:37

KristofferC force-pushed the kc/sparse_check branch from 9200e9b to d35d80b Compare June 25, 2017 18:24

Sacha0 reviewed Jun 26, 2017

View reviewed changes

Sacha0 requested changes Jun 26, 2017

View reviewed changes

implement an isvalid function for sparse matrices and use it in show

8e2297d

KristofferC force-pushed the kc/sparse_check branch from d35d80b to 8e2297d Compare June 27, 2017 06:18

KristofferC changed the title ~~implement an isvalid function for sparse matrices and use it in show~~ implement a checkvalid function for sparse matrices and use it in show Jun 27, 2017

tkelman reviewed Jun 27, 2017

View reviewed changes

rename SparseArrayInvalid and remove code block

58ff50c

Update sparsematrix.jl

33adca9

Sacha0 reviewed Jun 27, 2017

View reviewed changes

Sacha0 approved these changes Jun 27, 2017

View reviewed changes

KristofferC added 2 commits June 27, 2017 20:26

fix signature in docs of check methods to ::Type{Bool} instead of Bool

0d89ae4

Merge branch 'kc/sparse_check' of https://github.com/JuliaLang/julia …

d047792

…into kc/sparse_check

tkelman reviewed Jun 28, 2017

View reviewed changes

add code quotes to err msgs

2db4ac3

KristofferC force-pushed the kc/sparse_check branch from cf968e8 to 2db4ac3 Compare June 28, 2017 05:50

Sacha0 reviewed Jun 28, 2017

View reviewed changes

Sacha0 approved these changes Jun 28, 2017

View reviewed changes

tkelman reviewed Jun 28, 2017

View reviewed changes

address comments

687e41f

tkelman reviewed Aug 5, 2017

View reviewed changes

ViralBShah mentioned this pull request Dec 17, 2018

SparseMatrixCSC() allows creating invalid matrices #14489

Closed

KristofferC mentioned this pull request Jun 19, 2019

Argument checks for SparseMatrixCSC constructors #31724

Merged

DilumAluthge added arrays [a, r, r, a, y, s] and removed sparse Sparse arrays labels Jan 14, 2022

ViralBShah closed this Jan 17, 2022

ViralBShah added the sparse Sparse arrays label Jan 17, 2022

DilumAluthge deleted the kc/sparse_check branch January 18, 2022 04:56



		"""
		checkvalid(Bool, S::SparseMatrixCSC; full = true) -> Bool

implement a checkvalid function for sparse matrices and use it in show #22529

implement a checkvalid function for sparse matrices and use it in show #22529

Conversation

KristofferC commented Jun 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jun 25, 2017

nalimilan commented Jun 25, 2017

KristofferC commented Jun 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC commented Jun 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

KristofferC commented Jun 27, 2017

KristofferC commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC commented Aug 5, 2017

Choose a reason for hiding this comment

ViralBShah commented Dec 17, 2018

ViralBShah commented Jun 26, 2019

ViralBShah commented Mar 4, 2020

KristofferC commented Apr 7, 2021

ViralBShah commented Apr 7, 2021

DilumAluthge commented Jan 14, 2022

ViralBShah commented Jan 17, 2022

KristofferC commented Jun 25, 2017 •

edited

Loading