Skip to content

Commit

Permalink
Throw an error if zero() is not defined for SparseMatrix type
Browse files Browse the repository at this point in the history
  • Loading branch information
@simonster
simonster committed May 23, 2015
1 parent 2fedc27 commit c590f00
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 8 deletions.
10 changes: 7 additions & 3 deletions base/sparse/sparsematrix.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ type SparseMatrixCSC{Tv,Ti<:Integer} <: AbstractSparseMatrix{Tv,Ti}
function SparseMatrixCSC(m::Integer, n::Integer, colptr::Vector{Ti}, rowval::Vector{Ti}, nzval::Vector{Tv})
m < 0 && throw(ArgumentError("number of rows (m) must be ≥ 0, got $m"))
n < 0 && throw(ArgumentError("number of columns (n) must be ≥ 0, got $n"))
try

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman May 24, 2015

Contributor

I have some cases of code that uses this constructor many times on small amounts of input data in an inner loop, I'd be worried about this try-catch slowing things down.

This comment has been minimized.

Sign in to view

Copy link
@simonster

simonster May 24, 2015

Author Member

I thought about that, but hoped it wouldn't be a concern. If we're okay with throwing an ordinary MethodError, we could just call zero(Tv). Other solutions include a staged function or just continuing not to check for this.

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman May 24, 2015

Contributor

To be fair I'd need to properly benchmark this proposed change which I haven't done yet, but based on instinct and experience exception-handling can be especially slow on Windows particularly. And there's usually a better way to do most things than using a try-catch.

I'm also not so sure we want to be so militant about this in the constructor, since SparseMatrixCSC can be a useful data structure even if you're not doing arithmetic on it that would need zero to be defined. I feel like the MethodError at the math operation use site that would be calling for zero is more of the right thing to do here, but there might be some other option.

This comment has been minimized.

Sign in to view

Copy link
@simonster

simonster May 24, 2015

Author Member

If we throw an error at use, I would rather throw something more explicit than a MethodError. I guess we should be able to do that with zero overhead with a staged function.
zero(Tv)
catch e
isa(e, MethodError) && throw(ArgumentError("cannot construct a SparseMatrixCSC{$Tv} because zero($Tv) is not defined"))
rethrow(e)
end
new(Int(m), Int(n), colptr, rowval, nzval)
end
end
Expand Down Expand Up @@ -184,9 +190,7 @@ convert{T}(::Type{AbstractMatrix{T}}, A::SparseMatrixCSC) = convert(SparseMatrix
convert(::Type{Matrix}, S::SparseMatrixCSC) = full(S)

function full{Tv}(S::SparseMatrixCSC{Tv})
# Handle cases where zero(Tv) is not defined but the array is dense.
# (Should we really worry about this?)
A = length(S) == nnz(S) ? Array(Tv, S.m, S.n) : zeros(Tv, S.m, S.n)
A = zeros(Tv, S.m, S.n)
for col = 1 : S.n, k = S.colptr[col] : (S.colptr[col+1]-1)
A[S.rowval[k], col] = S.nzval[k]
end
Expand Down
6 changes: 4 additions & 2 deletions test/random.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,10 @@ for rng in ([], [MersenneTwister()], [RandomDevice()])
X = T == Bool ? T[0,1] : T[0,1,2]
rand!(rng..., A) ::typeof(A)
rand!(rng..., A, X) ::typeof(A)
rand!(rng..., sparse(A)) ::typeof(sparse(A))
rand!(rng..., sparse(A), X) ::typeof(sparse(A))
if T != Char
rand!(rng..., sparse(A)) ::typeof(sparse(A))
rand!(rng..., sparse(A), X) ::typeof(sparse(A))
end
end
end
end
Expand Down
5 changes: 2 additions & 3 deletions test/sparsedir/sparse.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -748,9 +748,8 @@ let S = spzeros(5,1), I = [false,true,false,true,false]
end

# issue #9917
@test sparse([]') == reshape(sparse([]), 1, 0)
@test full(sparse([])) == zeros(0, 1)

This comment has been minimized.

Sign in to view

Copy link
@tkelman

tkelman May 24, 2015

Contributor

can this test still be here, just for a type where zero is defined?

This comment has been minimized.

Sign in to view

Copy link
@simonster

simonster May 24, 2015

Author Member

I added it in the previous commit, and it never actually failed for types for which zero is defined. But I guess it doesn't hurt to have.
@test_throws BoundsError sparse([])[1]
@test sparse(Int[]') == reshape(sparse(Int[]), 1, 0)
@test_throws BoundsError sparse(Int[])[1]
x = speye(100)
@test_throws BoundsError x[-10:10]

Expand Down

1 comment on commit c590f00

@lindahua
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should allow the construction of arrays of element type T which does not admit a zero method. Such matrices can still be used in many methods, where zero is not invoked.

Please sign in to comment.