Skip to content

Simplify indexing #10

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Jan 3, 2021
Merged

Simplify indexing #10

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ad0e7e8
simplify indexing
Nov 16, 2020
5e1846a
linear indexing test
Nov 16, 2020
8a049e7
use fast linear indexing when possible
Nov 17, 2020
1329a79
cosmetic changes
Nov 17, 2020
e31f590
simplify linear indexing, and checkbounds
Nov 17, 2020
a549b56
Integer -> Int
Nov 18, 2020
c3f15db
one solution
Nov 18, 2020
a3f3273
work via to_indices instead
Nov 18, 2020
bcbdc81
some evil test cases
Nov 22, 2020
f71bf96
some inconclusive messing with to_indices
Nov 22, 2020
a4d79af
Fixed cartesian indexing
Vexatos Nov 22, 2020
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
45 changes: 36 additions & 9 deletions src/CircularArrays.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export CircularArray, CircularVector

`N`-dimensional array backed by an `AbstractArray{T, N}` of type `A` with fixed size and circular indexing.

array[index] == array[mod1(index, size)]
array[index...] == array[mod1.(index, size)...]
"""
struct CircularArray{T, N, A} <: AbstractArray{T, N}
data::A
Expand All @@ -23,24 +23,43 @@ end
One-dimensional array backed by an `AbstractArray{T, 1}` of type `A` with fixed size and circular indexing.
Alias for [`CircularArray{T,1,A}`](@ref).

array[index] == array[mod1(index, size)]
array[index] == array[mod1(index, length)]
"""
const CircularVector{T} = CircularArray{T, 1}

@inline clamp_bounds(arr::CircularArray, I::Tuple{Vararg{Int}})::AbstractArray{Int, 1} = map(Base.splat(mod), zip(I, axes(arr.data)))

CircularArray(data::AbstractArray{T,N}) where {T,N} = CircularArray{T,N}(data)
CircularArray{T}(data::AbstractArray{T,N}) where {T,N} = CircularArray{T,N}(data)
CircularArray(def::T, size) where T = CircularArray(fill(def, size))

@inline Base.getindex(arr::CircularArray, i::Int) = @inbounds getindex(arr.data, mod(i, Base.axes1(arr.data)))
@inline Base.setindex!(arr::CircularArray, v, i::Int) = @inbounds setindex!(arr.data, v, mod(i, Base.axes1(arr.data)))
@inline Base.getindex(arr::CircularArray, I::Vararg{Int}) = @inbounds getindex(arr.data, clamp_bounds(arr, I)...)
@inline Base.setindex!(arr::CircularArray, v, I::Vararg{Int}) = @inbounds setindex!(arr.data, v, clamp_bounds(arr, I)...)
@inline Base.getindex(arr::CircularArray, i::Int) =
@inbounds getindex(arr.data, mod1(i, length(arr.data)))
@inline Base.getindex(arr::CircularArray{T,N}, I::Vararg{<:Int,N}) where {T,N} =
@inbounds getindex(arr.data, map(mod, I, axes(arr.data))...)

@inline Base.setindex!(arr::CircularArray, v, i::Int) =
@inbounds setindex!(arr.data, v, mod1(i, length(arr.data)))
@inline Base.setindex!(arr::CircularArray{T,N}, v, I::Vararg{<:Int,N}) where {T,N} =
@inbounds setindex!(arr.data, v, map(mod, I, axes(arr.data))...)

@inline Base.size(arr::CircularArray) = size(arr.data)
@inline Base.axes(arr::CircularArray) = axes(arr.data)
Base.parent(arr::CircularArray) = arr.data

@inline function Base.to_indices(arr::CircularArray, ax, I::Tuple{CartesianIndex{M}, Vararg{Any}}) where {M}
J = ntuple(d -> mod(I[1].I[d], ax[d]), Val(M))
Base.to_indices(arr, ax[M:end], (J..., Base.tail(I)...))
end
@inline Base.to_indices(arr::CircularArray, I::Tuple{Vararg{Union{Integer, CartesianIndex}}}) =
to_indices(arr, axes(arr), I) # this method would normally omit axes, but we need them.
# and then these two are needed to solve ambiguity:
@inline Base.to_indices(arr::CircularArray, I::Tuple{Vararg{Int}}) = I
@inline Base.to_indices(arr::CircularArray, I::Tuple{Vararg{Integer}}) = to_indices(arr, (), I)

@inline Base.checkbounds(::CircularArray, _...) = nothing
@inline function Base.checkbounds(arr::CircularArray, I...)
J = Base.to_indices(arr, I)
length(J) == 1 || length(J) >= ndims(arr) || throw(BoundsError(arr, I))
nothing
end

@inline _similar(arr::CircularArray, ::Type{T}, dims) where T = CircularArray(similar(arr.data,T,dims))
@inline Base.similar(arr::CircularArray, ::Type{T}, dims::Tuple{Base.DimOrInd, Vararg{Base.DimOrInd}}) where T = _similar(arr,T,dims)
Expand All @@ -50,6 +69,14 @@ CircularArray(def::T, size) where T = CircularArray(fill(def, size))
CircularVector(data::AbstractArray{T, 1}) where T = CircularVector{T}(data)
CircularVector(def::T, size::Int) where T = CircularVector{T}(fill(def, size))

Base.IndexStyle(::Type{CircularArray{T,N,A}}) where {T,N,A} = IndexStyle(A)
Copy link
Owner

@Vexatos Vexatos Nov 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The question came to me why this line would be needed. Wouldn't the issues with CartesianIndex be resolved if the IndexStyle defaulted to IndexCartesian for anyhting but CircularVector? That would result in the calls with CartesianIndex parameters to run through the second implementation of getindex instead of the first. Then all the hacks into to_indices could also be removed. I would be very interested to see your "more evil test cases" and see if they work with this, because all the current tests pass like this.

Copy link
Author

@mcabbott mcabbott Nov 22, 2020

Choose a reason for hiding this comment

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

I thought this was necessary for speed, although it's entirely possible I have got lost in all the versions now. It causes eachindex(A) to prefer linear indexing, which can drop straight through to the underlying memory, whereas iterating CartesianIndices involves some multiplications by strides... necessary when the underlying array is transposed, say.

I've pushed what I had lying around.

Base.IndexStyle(::Type{<:CircularVector}) = IndexLinear()

function Base.showarg(io::IO, arr::CircularArray, toplevel)
print(io, ndims(arr) == 1 ? "CircularVector(" : "CircularArray(")
Base.showarg(io, parent(arr), false)
print(io, ')')
# toplevel && print(io, " with eltype ", eltype(arr))
end

end
30 changes: 30 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,25 @@ end
a1 = CircularArray(b_arr)
@test size(a1) == (3, 4)
@test a1[2, 3] == 14
@test a1[2, Int32(3)] == 14
a1[2, 3] = 17
@test a1[2, 3] == 17
@test a1[-1, 7] == 17
@test a1[CartesianIndex(-1, 7)] == 17
@test a1[-1:5, 4:10][1, 4] == 17
@test a1[:, -1:-1][2, 1] == 17
a1[CartesianIndex(-2, 7)] = 99
@test a1[1, 3] == 99

@test IndexStyle(a1) == IndexLinear()
@test a1[3] == a1[3,1]
@test a1[Int32(4)] == a1[1,2]
@test a1[-1] == a1[length(a1)-1]

@test a1[2, 3, 1] == 17 # trailing index
@test a1[2, 3, 99] == 17
@test a1[2, 3, :] == [17]

@test !isa(a1, CircularVector)
@test !isa(a1, AbstractVector)
@test isa(a1, AbstractArray)
Expand All @@ -80,6 +94,22 @@ end
@test isa(a2, CircularArray{Int, 2})
end

@testset "3-array" begin
t3 = collect(reshape('a':'x', 2,3,4))
c3 = CircularArray(t3)

@test c3[1,3,3] == c3[3,3,3] == c3[3,3,7]

c3[3,3,7] = 'Z'
@test t3[1,3,3] == 'Z'

@test IndexStyle(c3) == IndexLinear()
@test c3[-1] == t3[length(t3)-1]

@test_throws BoundsError c3[2,3] # too few indices
@test_throws BoundsError c3[CartesianIndex(2,3)]
end

@testset "offset indices" begin
i = OffsetArray(1:5,-3)
a = CircularArray(i)
Expand Down