Remove ragged field, and go back to simple AbstractArray interface.

Author: gabrielgellner

src/vector_of_array.jl

@@ -1,65 +1,49 @@
-abstract AbstractVectorOfArray{T, N} <: AbstractArray{T, N}
-
 # Based on code from M. Bauman Stackexchange answer + Gitter discussion
-type VectorOfArray{T, N, A} <: AbstractVectorOfArray{T, N}
+
+type VectorOfArray{T, N, A} <: AbstractArray{T, N}
   data::A # A <: AbstractVector{<: AbstractArray{T, N - 1}}
+  #TODO: I don't really use dims, how do I get rid of it, and still have everything work?
   dims::NTuple{N, Int}
-  ragged::Bool
 end
 
 function VectorOfArray(vec::AbstractVector)
-  # Only allow a vector of equally shaped subtypes, this will not work for ragged arrays
-  ragged = !all(size(vec[1]) == size(v) for v in vec)
-  VectorOfArray(vec, (size(vec[1])..., length(vec)), ragged)
+    # Only allow a vector of equally shaped subtypes, this will not work for ragged arrays
+    #@assert all(size(vec[1]) == size(v) for v in vec)
+    VectorOfArray(vec, (size(vec[1])..., length(vec)))
 end
 
-VectorOfArray{T, N}(vec::AbstractVector{T}, dims::NTuple{N}, ragged::Bool) = VectorOfArray{eltype(T), N, typeof(vec)}(vec, dims, ragged)
-@inline function Base.size(S::AbstractVectorOfArray)
-  if S.ragged
-    return size(S.data)
-  else
-    return S.dims
-  end
-end
+VectorOfArray{T, N}(vec::AbstractVector{T}, dims::NTuple{N}) = VectorOfArray{eltype(T), N, typeof(vec)}(vec, dims)
+Base.size(S::VectorOfArray) = (size(S.data[1])..., length(S.data))
 
-@inline function Base.getindex{T, N}(S::AbstractVectorOfArray{T, N}, I::Vararg{Int, N})
-  @boundscheck checkbounds(S, I...)
-  S.ragged && throw(BoundsError("A ragged VectorOfArray does not support Cartesian indexing"))
-  S.data[I[end]][Base.front(I)...]
+@inline function Base.getindex{T, N}(S::VectorOfArray{T, N}, I::Vararg{Int, N})
+    @boundscheck checkbounds(S, I...) # is this needed?
+    S.data[I[end]][Base.front(I)...]
 end
 # Linear indexing will be over the container elements, not the individual elements
 # unlike an true AbstractArray
-@inline Base.getindex{T, N}(S::AbstractVectorOfArray{T, N}, I::Union{Int, Colon, AbstractArray{Int}}) = S.data[I]
+@inline Base.getindex{T, N}(S::VectorOfArray{T, N}, I::Int) = S.data[I]
+@inline Base.getindex{T, N}(S::VectorOfArray{T, N}, I::AbstractArray{Int}) = S.data[I]
 
-Base.copy(S::AbstractVectorOfArray) = VectorOfArray(copy(S.data), S.dims, S.ragged)
+Base.copy(S::VectorOfArray) = VectorOfArray(copy(S.data), S.dims)
 
-Base.sizehint!{T, N}(S::AbstractVectorOfArray{T, N}, i) = sizehint!(S.data, i)
+Base.sizehint!{T, N}(S::VectorOfArray{T, N}, i) = sizehint!(S.data, i)
 
-function Base.push!{T, N}(S::AbstractVectorOfArray{T, N}, new_item::AbstractArray)
-  if S.dims[1:(end - 1)] == size(new_item)
-    S.dims = (S.dims[1:(end - 1)]..., S.dims[end] + 1)
-  else
-    S.ragged = true
-    # just revert down to a vector of elements, make the dims data meaningless
-    #TODO: this is stupid ugly
-    S.dims = tuple(-ones(Int, N)...)
-  end
-  push!(S.data, copy(new_item))
+function Base.push!{T, N}(S::VectorOfArray{T, N}, new_item::AbstractVector)
+    #@assert S.dims[1:(end - 1)] == size(new_item)
+    #S.dims = (S.dims[1:(end - 1)]..., S.dims[end] + 1)
+    push!(S.data, new_item)
 end
-function Base.append!{T, N}(S::AbstractVectorOfArray{T, N}, new_item::AbstractVectorOfArray{T, N})
+
+function Base.append!{T, N}(S::VectorOfArray{T, N}, new_item::VectorOfArray{T, N})
     for item in copy(new_item)
-      push!(S, item)
+        push!(S, item)
     end
     return S
 end
 
+
 # The iterator will be over the subarrays of the container, not the individual elements
 # unlike an true AbstractArray
-Base.start{T, N}(S::AbstractVectorOfArray{T, N}) = 1
-Base.next{T, N}(S::AbstractVectorOfArray{T, N}, state) = (S[state], state + 1)
-Base.done{T, N}(S::AbstractVectorOfArray{T, N}, state) = state >= length(S.data) + 1
-
-# convert to a regular, sense array
-function vecarr_to_arr(va::AbstractVectorOfArray)
-  va[[Colon() for d in 1:length(va.dims)]...]
-end
+Base.start{T, N}(S::VectorOfArray{T, N}) = 1
+Base.next{T, N}(S::VectorOfArray{T, N}, state) = (S[state], state + 1)
+Base.done{T, N}(S::VectorOfArray{T, N}, state) = state >= length(S.data) + 1

test/basic_indexing.jl

@@ -1,17 +1,24 @@
+using RecursiveArrayTools
 
 # Example Problem
 recs = [[1, 2, 3], [4, 5, 6], [7, 8, 9]]
 testa = cat(2, recs...)
 testva = VectorOfArray(recs)
-testa[1:2, 1:2] == [1 2; 4 5]
-testva[1:2, 1:2] == [1 2; 4 5]
+testa[1:2, 1:2] == [1 4; 2 5]
+testva[1:2, 1:2] == [1 4; 2 5]
 testa[1:2, 1:2] == [1 4; 2 5]
 
 # # ndims == 2
 recs = [rand(8) for i in 1:10]
 testa = cat(2, recs...)
 testva = VectorOfArray(recs)
 
+# ## Linear indexing
+testva[1] == testa[:, 1]
+testva[:] == recs
+testva[end] == testa[:, end]
+testva[2:end] == [recs[i] for i = 2:length(recs)]
+
 # ## (Int, Int)
 @test testa[5, 4] == testva[5, 4]
 
@@ -49,3 +56,8 @@ testva = VectorOfArray(recs)
 
 # ## (Range, Range, Range)
 @test testa[2:3, 2:3, 1:2] == testva[2:3, 2:3, 1:2]
+
+# ## Make sure that 1:1 like ranges are not collapsed
+@test testa[1:1, 2:3, 1:2] == testva[1:1, 2:3, 1:2]
+
+# ## Test ragged arrays work, or give errors as needed

test/interface_tests.jl

@@ -20,7 +20,7 @@ append!(testva, testva)
 # Test that adding a array of different dimension triggers the ragged flag
 push!(testva, [-1, -2, -3, -4])
 #testva #TODO: this screws up printing, try to make a fallback
-testva.ragged
+#testva.ragged
 @test_throws BoundsError testva[1:2, 5:6]
 @test testva[9] == [-1, -2, -3, -4]
 testva[end]