-
-
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
make sparse arrays print more consistently, also add some doctests #20488
Conversation
3becc15
to
94fdbab
Compare
I discussed a bit with @fredrikekre and will update soon with the new proposal. |
I propose this instead (consistent with julia> sprand(2, 2, 1.0)
2×2 SparseMatrixCSC{Float64,Int64} with 4 stored entries:
[1, 1] = 0.0506357
[2, 1] = 0.143032
[1, 2] = 0.932698
[2, 2] = 0.906617
julia> sprand(2, 1.0)
2-element SparseVector{Float64,Int64} with 2 stored entries:
[1] = 0.194626
[2] = 0.264848 |
94fdbab
to
5526b5f
Compare
Updated top post. |
base/sparse/sparsevector.jl
Outdated
[1] = 1.0 | ||
|
||
julia> Base.SparseArrays.dropstored!(x, 2) | ||
3-element SparseVector{Float64,Int64} with 1 stored entries: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this not show as entry
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, just messed up with the doctesting
97258a3
to
03d904a
Compare
Got an exception of type MethodError outside of a @test
MethodError: no method matching isassigned(::Array{Float64,1}, ::Int64)
Closest candidates are:
isassigned{T}(::Array{T,N} where N, !Matched::Int32...) at array.jl:67
isassigned(::AbstractArray, !Matched::Int32...) at abstractarray.jl:213 So is |
03d904a
to
d90276a
Compare
base/sparse/sparsematrix.jl
Outdated
@@ -117,7 +117,9 @@ column. In conjunction with [`nonzeros`](@ref) and | |||
nzrange(S::SparseMatrixCSC, col::Integer) = S.colptr[col]:(S.colptr[col+1]-1) | |||
|
|||
function Base.show(io::IO, ::MIME"text/plain", S::SparseMatrixCSC) | |||
print(io, S.m, "×", S.n, " sparse matrix with ", nnz(S), " ", eltype(S), " stored entries") | |||
xnnz = nnz(S) | |||
print(io, S.m, "×", S.n, " ", typeof(S), " with ", nnz(S), " stored ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why nnz(S)
the first time and xnnz
(= nnz(S)
) the second?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reminder. Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful printing!
base/sparse/sparsematrix.jl
Outdated
|
||
julia> Base.SparseArrays.dropstored!(A, 1, 2); A | ||
2×2 SparseMatrixCSC{Int64,Int64} with 1 stored entry: | ||
[1, 2] = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't this supposed to drop A[1, 2]
? is there an off-by-one in what dropstored!
does to the colptr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch --- dropstored!
might be broken?
julia> foo = sprand(4, 4, 0.5)
4×4 sparse matrix with 10 Float64 stored entries:
[2, 1] = 0.986522
[1, 2] = 0.331456
[2, 2] = 0.181261
[4, 2] = 0.545928
[1, 3] = 0.864019
[2, 3] = 0.48957
[3, 3] = 0.575748
[4, 3] = 0.509864
[1, 4] = 0.368723
[3, 4] = 0.332495
julia> Base.SparseArrays.dropstored!(foo, 2, 1)
4×4 sparse matrix with 9 Float64 stored entries:Error showing value of type SparseMatrixCSC{Float64,Int64}:
ERROR: BoundsError: attempt to access 9-element Array{Int64,1} at index [0]
Stacktrace:
[1] show(::IOContext{Base.Terminals.TTYTerminal}, ::SparseMatrixCSC{Float64,Int64}) at ./sparse/sparsematrix.jl:147
[2] show(::IOContext{Base.Terminals.TTYTerminal}, ::MIME{Symbol("text/plain")}, ::SparseMatrixCSC{Float64,Int64}) at ./sparse/sparsematrix.jl:122
[3] display(::Base.REPL.REPLDisplay{Base.REPL.LineEditREPL}, ::MIME{Symbol("text/plain")}, ::SparseMatrixCSC{Float64,Int64}) at ./REPL.jl:122
[4] display(::Base.REPL.REPLDisplay{Base.REPL.LineEditREPL}, ::SparseMatrixCSC{Float64,Int64}) at ./REPL.jl:125
[5] display(::SparseMatrixCSC{Float64,Int64}) at ./multimedia.jl:194
julia> foo.colptr
5-element Array{Int64,1}:
0
1
4
8
10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(#20513)
base/sparse/sparsevector.jl
Outdated
|
||
Create a sparse vector of length `m` where the nonzero indices are keys from | ||
the dictionary, and the nonzero values are the values from the dictionary. | ||
|
||
```jldoctest | ||
julia> sparsevec(Dict(1 => 3, 2 => 2, 2 => 4)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a minute I thought "oh that's interesting, sparsevec from a Dict doesn't do combining" but no, the overwriting happens in the Dict constructor so the duplicate key is probably confusing to have here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
d90276a
to
14d79c2
Compare
Does anyone understand where this error on 32 bit comes from: Error in testset sparse/sparse:
Error During Test
Got an exception of type MethodError outside of a @test
MethodError: no method matching isassigned(::Array{Float64,1}, ::Int64)
Closest candidates are:
isassigned(::Array{T,N} where N, !Matched::Int32...) where T at array.jl:67
isassigned(::AbstractArray, !Matched::Int32...) at abstractarray.jl:213
Stacktrace:
[1] similar(::Type{T} where T, ::Tuple{Base.OneTo{Int32}}) at ./abstractarray.jl:0
[2] copy!(::AbstractArray, ::Any) at ./abstractarray.jl:0
[3] inlineable(::Any, ::Any, ::Expr, ::Array{Any,1}, ::Core.Inference.InferenceState) at ./inference.jl:3840
[4] _setindex!(::Dict{Symbol,Any}, ::Set{Char}, ::Symbol, ::Int32) at ./dict.jl:396
[5] _setindex!(::Dict{Symbol,Any}, ::Set{Char}, ::Symbol, ::Int32) at ./dict.jl:403
[6] #show#7(::Bool, ::Function, ::IOContext{Base.AbstractIOBuffer{Array{UInt8,1}}}, ::StackFrame) at ./stacktraces.jl:0
[7] (::Base.#kw##show_trace_entry)(::Array{Any,1}, ::Base.#show_trace_entry, ::IOContext{Base.AbstractIOBuffer{Array{UInt8,1}}}, ::StackFrame, ::Int32) at ./<missing>:0 (repeats 2 times)
[8] showerror(::IOContext{Base.AbstractIOBuffer{Array{UInt8,1}}}, ::MethodError) at ./replutil.jl:292
[9] showerror(::IOContext{Base.AbstractIOBuffer{Array{UInt8,1}}}, ::MethodError) at ./replutil.jl:303
[10] show_method_candidates(::IOContext{Base.AbstractIOBuffer{Array{UInt8,1}}}, ::MethodError, ::Array{Any,1}) at ./replutil.jl:503 I added a workaround for now: e87c7f2 but I don't understand why this happens just for this case and not for printing any other array types. |
I feel this could be merged if no one has a problem with the e87c7f2 commit. |
worth looking into why that's necessary |
The stacktrace for that error (above) seems bonkers.. It's trying to show a the closest candidates for a MethodError. But what would throw a MethodError. And then have the build succeed when I define the |
Backtrace is better with julia-debug. The julia/base/sparse/sparsematrix.jl Line 152 in 360d30a
k to Int(k) on that line instead of adding isassigned methods.
|
Cool, thanks! I'll update it and run through CI. What do you think about a PR for this method for |
Good question (mostly a separate one from this PR though), I guess it depends on whether you think of the "canonical index" to be |
base/sparse/sparsevector.jl
Outdated
limit::Bool = get(io, :limit, false) | ||
half_screen_rows = limit ? div(displaysize(io)[1] - 8, 2) : typemax(Int) | ||
pad = ndigits(n) | ||
sep = "\n\t" | ||
if !haskey(io, :compact) | ||
io = IOContext(io, :compact => true) | ||
end | ||
for k = 1:length(nzind) | ||
if k < half_screen_rows || k > xnnz - half_screen_rows | ||
print(io, " ", '[', rpad(nzind[k], pad), "] = ") | ||
Base.show(io, nzval[k]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess this should probably also be
if isassigned(nzval, Int(k))
Base.show(io, nzval[k])
else
print(io, Base.undef_ref_str)
end
there just aren't tests of putting a possibly-undef type in as the values of a SparseVector and showing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref #12966
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, sparse vector and matrix doesnt do the same here:
julia> A = speye(3)
3×3 SparseMatrixCSC{Float64,Int64} with 3 stored entries:
[1, 1] = 1.0
[2, 2] = 1.0
[3, 3] = 1.0
julia> similar(A, T12960)
3×3 SparseMatrixCSC{T12960,Int64} with 3 stored entries:
[1, 1] = #undef
[2, 2] = #undef
[3, 3] = #undef
julia> a = sparsevec([1, 2, 3])
3-element SparseVector{Int64,Int64} with 3 stored entries:
[1] = 1
[2] = 2
[3] = 3
julia> similar(a, T12960)
3-element SparseVector{T12960,Int64} with 0 stored entries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something else I found:
julia> a = speye(3,3)
3×3 SparseMatrixCSC{Float64,Int64} with 3 stored entries:
[1, 1] = 1.0
[2, 2] = 1.0
[3, 3] = 1.0
julia> b = similar(a, Float64, Int64)
3×3 SparseMatrixCSC{Float64,Int64} with 3 stored entries:
[1, 1] = 6.92397e-310
[2, 2] = 6.92397e-310
[3, 3] = 6.92397e-310
julia> Base.SparseArrays.dropstored!(a, 1, 1)
3×3 SparseMatrixCSC{Float64,Int64} with 2 stored entries:
[2, 2] = 1.0
[3, 3] = 1.0
julia> b
3×3 SparseMatrixCSC{Float64,Int64} with 2 stored entries:
[2, 2] = 6.92397e-310
[3, 3] = 6.92397e-310
a
and b
alias their internal representation. Seems odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like I've complained about that partial aliasing somewhere before. Sacha might remember.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like I've complained about that partial aliasing somewhere before. Sacha might remember.
Yes, and with convert
:). The dormant #17507 targets that aliasing issue's incarnation for similar
. Best!
pushed some updates with regards to the previous discussion |
base/sparse/sparsevector.jl
Outdated
@@ -758,7 +760,11 @@ function show(io::IOContext, x::AbstractSparseVector) | |||
for k = 1:length(nzind) | |||
if k < half_screen_rows || k > xnnz - half_screen_rows | |||
print(io, " ", '[', rpad(nzind[k], pad), "] = ") | |||
Base.show(io, nzval[k]) | |||
if isassigned(nzval, Int(k)) | |||
show(io, nzval[k]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent is off (sorry if you copy pasted that from my example?)
base/sparse/sparsevector.jl
Outdated
@@ -35,6 +35,8 @@ count(x::SparseVector) = count(x.nzval) | |||
nonzeros(x::SparseVector) = x.nzval | |||
nonzeroinds(x::SparseVector) = x.nzind | |||
|
|||
similar(x::SparseVector, Tv::Type=eltype(x)) = SparseVector(x.n, copy(x.nzind), Array{Tv}(length(x.nzval))) | |||
similar{Tv,Ti}(x::SparseVector, ::Type{Tv}, ::Type{Ti}) = SparseVector(x.n, copy!(similar(x.nzind, Ti), x.nzind), copy!(similar(x.nzval, Tv), x.nzval)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting long for a single line
test/sparse/sparse.jl
Outdated
@@ -1729,3 +1729,12 @@ end | |||
show(io, MIME"text/plain"(), spzeros(Float32, Int64, 2, 2)) | |||
@test String(take!(io)) == "2×2 SparseMatrixCSC{Float32,Int64} with 0 stored entries" | |||
end | |||
|
|||
@testset "similar aliasing" begin | |||
a = sparse(rand(3,3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rand can technically return exactly 0 in very rare cases, can't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I tried to make it happen but it never did.
Updated |
test/sparse/sparsevector.jl
Outdated
type t20488 end | ||
|
||
@testset "similar" begin | ||
x = sparsevec(rand(3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done on both
test/sparse/sparsevector.jl
Outdated
@test String(take!(io)) == "1-element SparseVector{Float64,Int64} with 1 stored entry:\n [1] = 1.0" | ||
show(io, MIME"text/plain"(), spzeros(Float64, Int64, 2)) | ||
@test String(take!(io)) == "2-element SparseVector{Float64,Int64} with 0 stored entries" | ||
show(io, similar(sparsevec(rand(3)), t20488)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
Currently, the printout is a bit all over the place for sparse arrays:
You can for example not see what type the indices are stored in. This PR makes it more consistent: