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

Improve error messages in abstractarray.jl #11797

Merged
merged 1 commit into from
Jul 14, 2015
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Improve error messages
Updated messages based on review

Update variable names per Matt

Updated tests to handle ArgumentError instead of BoundsError when passed iterable

More specific exceptions for test_throws

Fix dangling right parenthesis

Update non-abstract array error messages and tests
  • Loading branch information
ScottPJones committed Jul 14, 2015
commit f47c9b7e490b8fcc48a29cca7219d8c6b41c45a1
137 changes: 90 additions & 47 deletions base/abstractarray.jl
Original file line number Diff line number Diff line change
@@ -62,7 +62,12 @@ ndims{T<:AbstractArray}(::Type{T}) = ndims(super(T))
length(t::AbstractArray) = prod(size(t))::Int
endof(a::AbstractArray) = length(a)
first(a::AbstractArray) = a[1]
first(a) = (s = start(a); done(a, s) ? throw(ArgumentError("collection must be non-empty")) : next(a, s)[1])

function first(itr)
state = start(itr)
done(itr, state) && throw(ArgumentError("collection must be non-empty"))
next(itr, state)[1]
end
last(a) = a[end]

function stride(a::AbstractArray, i::Integer)
@@ -154,11 +159,13 @@ checkbounds(A::AbstractArray, I::AbstractVector{Bool}) = length(A) == length(I)
checkbounds(A::AbstractArray, I::Union{Real,AbstractArray,Colon}) = (@_inline_meta; _checkbounds(length(A), I) || throw_boundserror(A, I))
function checkbounds(A::AbstractMatrix, I::Union{Real,AbstractArray,Colon}, J::Union{Real,AbstractArray,Colon})
@_inline_meta
(_checkbounds(size(A,1), I) && _checkbounds(size(A,2), J)) || throw_boundserror(A, (I, J))
(_checkbounds(size(A,1), I) && _checkbounds(size(A,2), J)) ||
throw_boundserror(A, (I, J))
end
function checkbounds(A::AbstractArray, I::Union{Real,AbstractArray,Colon}, J::Union{Real,AbstractArray,Colon})
@_inline_meta
(_checkbounds(size(A,1), I) && _checkbounds(trailingsize(A,Val{2}), J)) || throw_boundserror(A, (I, J))
(_checkbounds(size(A,1), I) && _checkbounds(trailingsize(A,Val{2}), J)) ||
throw_boundserror(A, (I, J))
end
@generated function checkbounds(A::AbstractArray, I::Union{Real,AbstractArray,Colon}...)
meta = Expr(:meta, :inline)
@@ -197,7 +204,7 @@ similar( a::AbstractArray, T, dims::Dims) = Array(T, dims)

function reshape(a::AbstractArray, dims::Dims)
if prod(dims) != length(a)
throw(ArgumentError("dimensions must be consistent with array size"))
throw(ArgumentError("dimensions must be consistent with array size (expected $(length(a)), got $(prod(dims)))"))
end
copy!(similar(a, dims), a)
end
@@ -216,32 +223,40 @@ end

# if src is not an AbstractArray, moving to the offset might be O(n)
function copy!(dest::AbstractArray, doffs::Integer, src)
doffs < 1 && throw(BoundsError())
doffs < 1 && throw(BoundsError(dest, doffs))
st = start(src)
i, dmax = doffs, length(dest)
@inbounds while !done(src, st)
i > dmax && throw(BoundsError())
i > dmax && throw(BoundsError(dest, i))
val, st = next(src, st)
dest[i] = val
i += 1
end
return dest
end

# copy from an some iterable object into an AbstractArray
function copy!(dest::AbstractArray, doffs::Integer, src, soffs::Integer)
if (doffs < 1) | (soffs < 1)
throw(BoundsError())
doffs < 1 && throw(BoundsError(dest, doffs))
throw(ArgumentError(string("source start offset (",soffs,") is < 1")))
end
st = start(src)
for j = 1:(soffs-1)
done(src, st) && throw(BoundsError())
if done(src, st)
throw(ArgumentError(string("source has fewer elements than required, ",
"expected at least ",soffs,", got ",j-1)))
end
_, st = next(src, st)
end
dn = done(src, st)
dn && throw(BoundsError())
if dn
throw(ArgumentError(string("source has fewer elements than required, ",
"expected at least ",soffs,", got ",soffs-1)))
end
i, dmax = doffs, length(dest)
@inbounds while !dn
i > dmax && throw(BoundsError())
i > dmax && throw(BoundsError(dest, i))
val, st = next(src, st)
dest[i] = val
i += 1
@@ -252,15 +267,20 @@ end

# this method must be separate from the above since src might not have a length
function copy!(dest::AbstractArray, doffs::Integer, src, soffs::Integer, n::Integer)
n < 0 && throw(BoundsError())
n < 0 && throw(BoundsError(dest, n))
n == 0 && return dest
dmax = doffs + n - 1
if (dmax > length(dest)) | (doffs < 1) | (soffs < 1)
throw(BoundsError())
doffs < 1 && throw(BoundsError(dest, doffs))
soffs < 1 && throw(ArgumentError(string("source start offset (",soffs,") is < 1")))
throw(BoundsError(dest, dmax))
end
st = start(src)
for j = 1:(soffs-1)
done(src, st) && throw(BoundsError())
if done(src, st)
throw(ArgumentError(string("source has fewer elements than required, ",
"expected at least ",soffs,", got ",j-1)))
end
_, st = next(src, st)
end
i = doffs
@@ -269,7 +289,7 @@ function copy!(dest::AbstractArray, doffs::Integer, src, soffs::Integer, n::Inte
dest[i] = val
i += 1
end
i <= dmax && throw(BoundsError())
i <= dmax && throw(BoundsError(dest, i))
return dest
end

@@ -278,9 +298,7 @@ end

function copy!(dest::AbstractArray, src::AbstractArray)
n = length(src)
if n > length(dest)
throw(BoundsError())
end
n > length(dest) && throw(BoundsError(dest, n))
@inbounds for i = 1:n
dest[i] = src[i]
end
@@ -290,16 +308,21 @@ end
function copy!(dest::AbstractArray, doffs::Integer, src::AbstractArray)
copy!(dest, doffs, src, 1, length(src))
end

function copy!(dest::AbstractArray, doffs::Integer, src::AbstractArray, soffs::Integer)
soffs > length(src) && throw(BoundsError())
soffs > length(src) && throw(BoundsError(src, soffs))
copy!(dest, doffs, src, soffs, length(src)-soffs+1)
end
function copy!(dest::AbstractArray, doffs::Integer, src::AbstractArray, soffs::Integer, n::Integer)
n < 0 && throw(BoundsError())

function copy!(dest::AbstractArray, doffs::Integer,
src::AbstractArray, soffs::Integer,
n::Integer)
n == 0 && return dest
if soffs+n-1 > length(src) || doffs+n-1 > length(dest) || doffs < 1 || soffs < 1
throw(BoundsError())
end
n < 0 && throw(BoundsError(src, n))
soffs+n-1 > length(src) && throw(BoundsError(src, soffs+n-1))
doffs+n-1 > length(dest) && throw(BoundsError(dest, doffs+n-1))
doffs < 1 && throw(BoundsError(dest, doffs))
soffs < 1 && throw(BoundsError(src, soffs))
@inbounds for i = 0:(n-1)
dest[doffs+i] = src[soffs+i]
end
@@ -308,9 +331,15 @@ end

copy(a::AbstractArray) = copy!(similar(a), a)

function copy!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{Int}, A::AbstractVecOrMat{S}, ir_src::Range{Int}, jr_src::Range{Int})
if length(ir_dest) != length(ir_src) || length(jr_dest) != length(jr_src)
throw(ArgumentError("source and destination must have same size"))
function copy!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{Int},
A::AbstractVecOrMat{S}, ir_src::Range{Int}, jr_src::Range{Int})
if length(ir_dest) != length(ir_src)
throw(ArgumentError(string("source and destination must have same size (got ",
length(ir_src)," and ",length(ir_dest),")")))
end
if length(jr_dest) != length(jr_src)
throw(ArgumentError(string("source and destination must have same size (got ",
length(jr_src)," and ",length(jr_dest),")")))
end
Copy link
Member

Choose a reason for hiding this comment

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

Same here — you can remove the if block.

checkbounds(B, ir_dest, jr_dest)
checkbounds(A, ir_src, jr_src)
@@ -326,9 +355,15 @@ function copy!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{
return B
end

function copy_transpose!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{Int}, A::AbstractVecOrMat{S}, ir_src::Range{Int}, jr_src::Range{Int})
if length(ir_dest) != length(jr_src) || length(jr_dest) != length(ir_src)
throw(ArgumentError("source and destination must have same size"))
function copy_transpose!{R,S}(B::AbstractVecOrMat{R}, ir_dest::Range{Int}, jr_dest::Range{Int},
A::AbstractVecOrMat{S}, ir_src::Range{Int}, jr_src::Range{Int})
if length(ir_dest) != length(jr_src)
throw(ArgumentError(string("source and destination must have same size (got ",
length(jr_src)," and ",length(ir_dest),")")))
end
if length(jr_dest) != length(ir_src)
throw(ArgumentError(string("source and destination must have same size (got ",
length(ir_src)," and ",length(jr_dest),")")))
end
checkbounds(B, ir_dest, jr_dest)
checkbounds(A, ir_src, jr_src)
@@ -690,7 +725,7 @@ function hcat{T}(A::AbstractVecOrMat{T}...)
for j = 1:nargs
Aj = A[j]
if size(Aj, 1) != nrows
throw(ArgumentError("number of rows must match"))
throw(ArgumentError("number of rows of each array must match (got $(map(x->size(x,1), A)))"))
end
dense &= isa(Aj,Array)
nd = ndims(Aj)
@@ -722,7 +757,7 @@ function vcat{T}(A::AbstractMatrix{T}...)
ncols = size(A[1], 2)
for j = 2:nargs
if size(A[j], 2) != ncols
throw(ArgumentError("number of columns must match"))
throw(ArgumentError("number of columns of each array must match (got $(map(x->size(x,2), A)))"))
end
end
B = similar(full(A[1]), nrows, ncols)
@@ -761,11 +796,13 @@ function cat_t(catdims, typeC::Type, X...)
for i = 2:nargs
for d = 1:ndimsC
currentdim = (d <= ndimsX[i] ? size(X[i],d) : 1)
if dims2cat[d]==0
dimsC[d] == currentdim || throw(DimensionMismatch("mismatch in dimension $(d)"))
else
if dims2cat[d] != 0
dimsC[d] += currentdim
catsizes[i,dims2cat[d]] = currentdim
elseif dimsC[d] != currentdim
throw(DimensionMismatch(string("mismatch in dimension ",d,
" (expected ",dimsC[d],
" got ",currentdim,")")))
end
end
end
@@ -777,7 +814,8 @@ function cat_t(catdims, typeC::Type, X...)

offsets = zeros(Int,length(catdims))
for i=1:nargs
cat_one = [ dims2cat[d]==0 ? (1:dimsC[d]) : (offsets[dims2cat[d]]+(1:catsizes[i,dims2cat[d]])) for d=1:ndimsC]
cat_one = [ dims2cat[d] == 0 ? (1:dimsC[d]) : (offsets[dims2cat[d]]+(1:catsizes[i,dims2cat[d]]))
for d=1:ndimsC ]
C[cat_one...] = X[i]
for k = 1:length(catdims)
offsets[k] += catsizes[i,k]
@@ -817,9 +855,8 @@ typed_hcat(T::Type, A::AbstractArray...) = cat_t(2, T, A...)
function hvcat(nbc::Integer, as...)
# nbc = # of block columns
n = length(as)
if mod(n,nbc) != 0
throw(ArgumentError("all rows must have the same number of block columns"))
end
mod(n,nbc) != 0 &&
throw(ArgumentError("number of arrays $n is not a multiple of the requested number of block columns $nbc"))
nbr = div(n,nbc)
hvcat(ntuple(i->nbc, nbr), as...)
end
@@ -850,16 +887,16 @@ function hvcat{T}(rows::Tuple{Vararg{Int}}, as::AbstractMatrix{T}...)
Aj = as[a+j-1]
szj = size(Aj,2)
if size(Aj,1) != szi
throw(ArgumentError("mismatched height in block row $(i)"))
throw(ArgumentError("mismatched height in block row $(i) (expected $szi, got $(size(Aj,1)))"))
end
if c-1+szj > nc
throw(ArgumentError("block row $(i) has mismatched number of columns"))
throw(ArgumentError("block row $(i) has mismatched number of columns (expected $nc, got $(c-1+szj))"))
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the != as below would be better (or at least be consistent)

Copy link
Member

Choose a reason for hiding this comment

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

Would be clearer to me to write block row $(i) has mismatched number of columns (expected $nc, got $(c-1+szj))

out[r:r-1+szi, c:c-1+szj] = Aj
c += szj
end
if c != nc+1
throw(ArgumentError("block row $(i) has mismatched number of columns"))
throw(ArgumentError("block row $(i) has mismatched number of columns (expected $nc, got $(c-1))"))
end
r += szi
a += rows[i]
@@ -875,12 +912,12 @@ function hvcat{T<:Number}(rows::Tuple{Vararg{Int}}, xs::T...)

a = Array(T, nr, nc)
if length(a) != length(xs)
throw(ArgumentError("argument count does not match specified shape"))
throw(ArgumentError("argument count does not match specified shape (expected $(length(a)), got $(length(xs)))"))
end
k = 1
@inbounds for i=1:nr
if nc != rows[i]
throw(ArgumentError("row $(i) has mismatched number of columns"))
throw(ArgumentError("row $(i) has mismatched number of columns (expected $nc, got $(rows[i]))"))
end
for j=1:nc
a[i,j] = xs[k]
@@ -907,7 +944,7 @@ function typed_hvcat(T::Type, rows::Tuple{Vararg{Int}}, xs::Number...)
nc = rows[1]
for i = 2:nr
if nc != rows[i]
throw(ArgumentError("row $(i) has mismatched number of columns"))
throw(ArgumentError("row $(i) has mismatched number of columns (expected $nc, got $(rows[i]))"))
end
end
len = length(xs)
@@ -1031,7 +1068,7 @@ function sub2ind{T<:Integer}(dims::Tuple{Vararg{Integer}}, I::AbstractVector{T}.
for i=1:M
indices[i] += s*(Ij[i]-1)
end
s*= (j <= N ? dims[j] : 1)
s *= (j <= N ? dims[j] : 1)
end
return indices
end
@@ -1125,15 +1162,21 @@ function map(f, iters...)
nxtvals = cell(len)
cont = true
for idx in 1:len
done(iters[idx], states[idx]) && (cont = false; break)
if done(iters[idx], states[idx])
cont = false
break
end
end
while cont
for idx in 1:len
nxtvals[idx],states[idx] = next(iters[idx], states[idx])
end
push!(result, f(nxtvals...))
for idx in 1:len
done(iters[idx], states[idx]) && (cont = false; break)
if done(iters[idx], states[idx])
cont = false
break
end
end
end
result
14 changes: 10 additions & 4 deletions test/copy.jl
Original file line number Diff line number Diff line change
@@ -21,14 +21,20 @@ for (dest, src, bigsrc, emptysrc, res) in [
@test copy!(copy(dest), 99, src(), 99, 0) == dest

@test copy!(copy(dest), 1, emptysrc()) == dest
@test_throws BoundsError copy!(dest, 1, emptysrc(), 1)
x = emptysrc()
exc = isa(x, AbstractArray) ? BoundsError : ArgumentError
@test_throws exc copy!(dest, 1, emptysrc(), 1)

for idx in [0, 4]
for idx in (0, 4)
@test_throws BoundsError copy!(dest, idx, src())
@test_throws BoundsError copy!(dest, idx, src(), 1)
@test_throws BoundsError copy!(dest, idx, src(), 1, 1)
@test_throws BoundsError copy!(dest, 1, src(), idx)
@test_throws BoundsError copy!(dest, 1, src(), idx, 1)
x = src()
exc = isa(x, AbstractArray) ? BoundsError : ArgumentError
@test_throws exc copy!(dest, 1, x, idx)
x = src()
exc = isa(x, AbstractArray) ? BoundsError : ArgumentError
@test_throws exc copy!(dest, 1, x, idx, 1)
end

@test_throws BoundsError copy!(dest, 1, src(), 1, -1)