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.

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

RFC: Speedier, simpler and more systematic index conversions #19730

Merged
merged 15 commits into from
Jan 13, 2017
Merged

RFC: Speedier, simpler and more systematic index conversions #19730

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8a55211
Simpler and more systematic index conversions
mbauman Dec 27, 2016
5650778
Attempt to address performance regressions
mbauman Dec 28, 2016
9b4bdef
More perf improvements
mbauman Dec 29, 2016
b85c1c7
Type stable concatenation
mbauman Dec 29, 2016
82f9cde
Specialize indexing into ::Array with mixtures of Integers and Cartes…
mbauman Dec 29, 2016
3992c77
Fix and test views with logical masks
mbauman Dec 30, 2016
7753e66
Revert "Type stable concatenation"
mbauman Jan 2, 2017
a5d45bb
Simpler workaround for concatenation type-stability
mbauman Jan 2, 2017
2b28d7a
Minor docstring fixups from review
mbauman Jan 2, 2017
fc4563e
Further optimize iteration over LogicalIndex
mbauman Jan 6, 2017
737a156
Better deprecation messages
mbauman Jan 6, 2017
b0e90e1
Export to_indices and expand its documentation
mbauman Jan 7, 2017
9cd522c
Remove local variable N in cat
mbauman Jan 7, 2017
748fd0e
More careful consideration of multidimensional logical indices
mbauman Jan 11, 2017
1b2b738
Add Colon to stdlib; touch up its doc
mbauman Jan 11, 2017
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
102 changes: 50 additions & 52 deletions base/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ false
checkindex(::Type{Bool}, inds::AbstractUnitRange, i) = throw(ArgumentError("unable to check bounds for indices of type $(typeof(i))"))
checkindex(::Type{Bool}, inds::AbstractUnitRange, i::Real) = (first(inds) <= i) & (i <= last(inds))
checkindex(::Type{Bool}, inds::AbstractUnitRange, ::Colon) = true
checkindex(::Type{Bool}, inds::AbstractUnitRange, ::Slice) = true
function checkindex(::Type{Bool}, inds::AbstractUnitRange, r::Range)
@_propagate_inbounds_meta
isempty(r) | (checkindex(Bool, inds, first(r)) & checkindex(Bool, inds, last(r)))
Expand Down Expand Up @@ -799,7 +800,6 @@ end
pointer{T}(x::AbstractArray{T}) = unsafe_convert(Ptr{T}, x)
pointer{T}(x::AbstractArray{T}, i::Integer) = (@_inline_meta; unsafe_convert(Ptr{T},x) + (i-first(linearindices(x)))*elsize(x))


## Approach:
# We only define one fallback method on getindex for all argument types.
# That dispatches to an (inlined) internal _getindex function, where the goal is
Expand All @@ -812,61 +812,62 @@ pointer{T}(x::AbstractArray{T}, i::Integer) = (@_inline_meta; unsafe_convert(Ptr

function getindex(A::AbstractArray, I...)
@_propagate_inbounds_meta
_getindex(linearindexing(A), A, I...)
error_if_canonical_indexing(linearindexing(A), A, I...)
_getindex(linearindexing(A), A, to_indices(A, I)...)
end
function unsafe_getindex(A::AbstractArray, I...)
@_inline_meta
@inbounds r = getindex(A, I...)
r
end

error_if_canonical_indexing(::LinearFast, A::AbstractArray, ::Int) = error("indexing not defined for ", typeof(A))
error_if_canonical_indexing{T,N}(::LinearSlow, A::AbstractArray{T,N}, ::Vararg{Int, N}) = error("indexing not defined for ", typeof(A))
error_if_canonical_indexing(::LinearIndexing, ::AbstractArray, ::Any...) = nothing

## Internal definitions
_getindex(::LinearIndexing, A::AbstractArray, I...) = error("indexing $(typeof(A)) with types $(typeof(I)) is not supported")

## LinearFast Scalar indexing: canonical method is one Int
_getindex(::LinearFast, A::AbstractVector, ::Int) = error("indexing not defined for ", typeof(A))
_getindex(::LinearFast, A::AbstractArray, ::Int) = error("indexing not defined for ", typeof(A))
_getindex(::LinearFast, A::AbstractVector, i::Int) = (@_propagate_inbounds_meta; getindex(A, i))
_getindex(::LinearFast, A::AbstractArray, i::Int) = (@_propagate_inbounds_meta; getindex(A, i))
_getindex{T}(::LinearFast, A::AbstractArray{T,0}) = A[1]
_getindex(::LinearFast, A::AbstractArray, i::Real) = (@_propagate_inbounds_meta; getindex(A, to_index(i)))
function _getindex{T,N}(::LinearFast, A::AbstractArray{T,N}, I::Vararg{Real,N})
function _getindex{T,N}(::LinearFast, A::AbstractArray{T,N}, I::Vararg{Int,N})
# We must check bounds for sub2ind; so we can then use @inbounds
@_inline_meta
J = to_indexes(I...)
@boundscheck checkbounds(A, J...)
@inbounds r = getindex(A, sub2ind(A, J...))
@boundscheck checkbounds(A, I...)
@inbounds r = getindex(A, sub2ind(A, I...))
r
end
function _getindex(::LinearFast, A::AbstractVector, I1::Real, I::Real...)
function _getindex(::LinearFast, A::AbstractVector, I1::Int, I::Int...)
@_inline_meta
J = to_indexes(I1, I...)
@boundscheck checkbounds(A, J...)
@inbounds r = getindex(A, J[1])
@boundscheck checkbounds(A, I1, I...)
@inbounds r = getindex(A, I1)
r
end
function _getindex(::LinearFast, A::AbstractArray, I::Real...) # TODO: DEPRECATE FOR #14770
function _getindex(::LinearFast, A::AbstractArray, I::Int...) # TODO: DEPRECATE FOR #14770
@_inline_meta
J = to_indexes(I...)
@boundscheck checkbounds(A, J...)
@inbounds r = getindex(A, sub2ind(A, J...))
@boundscheck checkbounds(A, I...)
@inbounds r = getindex(A, sub2ind(A, I...))
r
end


## LinearSlow Scalar indexing: Canonical method is full dimensionality of Ints
_getindex{T,N}(::LinearSlow, A::AbstractArray{T,N}, ::Vararg{Int, N}) = error("indexing not defined for ", typeof(A))
_getindex{T,N}(::LinearSlow, A::AbstractArray{T,N}, I::Vararg{Real, N}) = (@_propagate_inbounds_meta; getindex(A, to_indexes(I...)...))
function _getindex(::LinearSlow, A::AbstractArray, i::Real)
_getindex{T,N}(::LinearSlow, A::AbstractArray{T,N}, I::Vararg{Int, N}) = (@_propagate_inbounds_meta; getindex(A, I...))
function _getindex(::LinearSlow, A::AbstractArray, i::Int)
# ind2sub requires all dimensions to be > 0; may as well just check bounds
@_inline_meta
@boundscheck checkbounds(A, i)
@inbounds r = getindex(A, ind2sub(A, to_index(i))...)
@inbounds r = getindex(A, ind2sub(A, i)...)
r
end
@generated function _getindex{T,AN}(::LinearSlow, A::AbstractArray{T,AN}, I::Real...) # TODO: DEPRECATE FOR #14770
@generated function _getindex{T,AN}(::LinearSlow, A::AbstractArray{T,AN}, I::Int...) # TODO: DEPRECATE FOR #14770
N = length(I)
if N > AN
# Drop trailing ones
Isplat = Expr[:(I[$d]) for d = 1:AN]
Osplat = Expr[:(to_index(I[$d]) == 1) for d = AN+1:N]
Osplat = Expr[:(I[$d] == 1) for d = AN+1:N]
quote
@_propagate_inbounds_meta
@boundscheck (&)($(Osplat...)) || throw_boundserror(A, I)
Expand All @@ -878,7 +879,7 @@ end
sz = Expr(:tuple)
sz.args = Expr[:(size(A, $d)) for d=max(N,1):AN]
szcheck = Expr[:(size(A, $d) > 0) for d=max(N,1):AN]
last_idx = N > 0 ? :(to_index(I[$N])) : 1
last_idx = N > 0 ? :(I[$N]) : 1
quote
# ind2sub requires all dimensions to be > 0:
@_propagate_inbounds_meta
Expand All @@ -892,7 +893,8 @@ end
# function that allows dispatch on array storage
function setindex!(A::AbstractArray, v, I...)
@_propagate_inbounds_meta
_setindex!(linearindexing(A), A, v, I...)
error_if_canonical_indexing(linearindexing(A), A, I...)
_setindex!(linearindexing(A), A, v, to_indices(A, I)...)
end
function unsafe_setindex!(A::AbstractArray, v, I...)
@_inline_meta
Expand All @@ -903,49 +905,44 @@ end
_setindex!(::LinearIndexing, A::AbstractArray, v, I...) = error("indexing $(typeof(A)) with types $(typeof(I)) is not supported")

## LinearFast Scalar indexing
_setindex!(::LinearFast, A::AbstractVector, v, ::Int) = error("indexed assignment not defined for ", typeof(A))
_setindex!(::LinearFast, A::AbstractArray, v, ::Int) = error("indexed assignment not defined for ", typeof(A))
_setindex!(::LinearFast, A::AbstractVector, v, i::Int) = (@_propagate_inbounds_meta; setindex!(A, v, i))
_setindex!(::LinearFast, A::AbstractArray, v, i::Int) = (@_propagate_inbounds_meta; setindex!(A, v, i))
_setindex!{T}(::LinearFast, A::AbstractArray{T,0}, v) = (@_propagate_inbounds_meta; setindex!(A, v, 1))
_setindex!(::LinearFast, A::AbstractArray, v, i::Real) = (@_propagate_inbounds_meta; setindex!(A, v, to_index(i)))
function _setindex!{T,N}(::LinearFast, A::AbstractArray{T,N}, v, I::Vararg{Real,N})
function _setindex!{T,N}(::LinearFast, A::AbstractArray{T,N}, v, I::Vararg{Int,N})
# We must check bounds for sub2ind; so we can then use @inbounds
@_inline_meta
J = to_indexes(I...)
@boundscheck checkbounds(A, J...)
@inbounds r = setindex!(A, v, sub2ind(A, J...))
@boundscheck checkbounds(A, I...)
@inbounds r = setindex!(A, v, sub2ind(A, I...))
r
end
function _setindex!(::LinearFast, A::AbstractVector, v, I1::Real, I::Real...)
function _setindex!(::LinearFast, A::AbstractVector, v, I1::Int, I::Int...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right that this signature is for allowing extra trailing 1's?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup.

@_inline_meta
J = to_indexes(I1, I...)
@boundscheck checkbounds(A, J...)
@inbounds r = setindex!(A, v, J[1])
@boundscheck checkbounds(A, I1, I...)
@inbounds r = setindex!(A, v, I1)
r
end
function _setindex!(::LinearFast, A::AbstractArray, v, I::Real...) # TODO: DEPRECATE FOR #14770
function _setindex!(::LinearFast, A::AbstractArray, v, I::Int...) # TODO: DEPRECATE FOR #14770
@_inline_meta
J = to_indexes(I...)
@boundscheck checkbounds(A, J...)
@inbounds r = setindex!(A, v, sub2ind(A, J...))
@boundscheck checkbounds(A, I...)
@inbounds r = setindex!(A, v, sub2ind(A, I...))
r
end

# LinearSlow Scalar indexing
_setindex!{T,N}(::LinearSlow, A::AbstractArray{T,N}, v, ::Vararg{Int, N}) = error("indexed assignment not defined for ", typeof(A))
_setindex!{T,N}(::LinearSlow, A::AbstractArray{T,N}, v, I::Vararg{Real, N}) = (@_propagate_inbounds_meta; setindex!(A, v, to_indexes(I...)...))
function _setindex!(::LinearSlow, A::AbstractArray, v, i::Real)
_setindex!{T,N}(::LinearSlow, A::AbstractArray{T,N}, v, I::Vararg{Int, N}) = (@_propagate_inbounds_meta; setindex!(A, v, I...))
function _setindex!(::LinearSlow, A::AbstractArray, v, i::Int)
# ind2sub requires all dimensions to be > 0; may as well just check bounds
@_inline_meta
@boundscheck checkbounds(A, i)
@inbounds r = setindex!(A, v, ind2sub(A, to_index(i))...)
@inbounds r = setindex!(A, v, ind2sub(A, i)...)
r
end
@generated function _setindex!{T,AN}(::LinearSlow, A::AbstractArray{T,AN}, v, I::Real...) # TODO: DEPRECATE FOR #14770
@generated function _setindex!{T,AN}(::LinearSlow, A::AbstractArray{T,AN}, v, I::Int...) # TODO: DEPRECATE FOR #14770
N = length(I)
if N > AN
# Drop trailing ones
Isplat = Expr[:(I[$d]) for d = 1:AN]
Osplat = Expr[:(to_index(I[$d]) == 1) for d = AN+1:N]
Osplat = Expr[:(I[$d] == 1) for d = AN+1:N]
quote
# We only check the trailing ones, so just propagate @inbounds state
@_propagate_inbounds_meta
Expand All @@ -958,7 +955,7 @@ end
sz = Expr(:tuple)
sz.args = Expr[:(size(A, $d)) for d=max(N,1):AN]
szcheck = Expr[:(size(A, $d) > 0) for d=max(N,1):AN]
last_idx = N > 0 ? :(to_index(I[$N])) : 1
last_idx = N > 0 ? :(I[$N]) : 1
quote
# ind2sub requires all dimensions to be > 0:
@_propagate_inbounds_meta
Expand Down Expand Up @@ -994,7 +991,7 @@ function get!{T}(X::AbstractArray{T}, A::AbstractArray, I::Union{Range, Abstract
X
end

get(A::AbstractArray, I::Range, default) = get!(similar(A, typeof(default), index_shape(A, I)), A, I, default)
get(A::AbstractArray, I::Range, default) = get!(similar(A, typeof(default), index_shape(I)), A, I, default)

# TODO: DEPRECATE FOR #14770 (just the partial linear indexing part)
function get!{T}(X::AbstractArray{T}, A::AbstractArray, I::RangeVecIntList, default::T)
Expand All @@ -1004,7 +1001,7 @@ function get!{T}(X::AbstractArray{T}, A::AbstractArray, I::RangeVecIntList, defa
X
end

get(A::AbstractArray, I::RangeVecIntList, default) = get!(similar(A, typeof(default), index_shape(A, I...)), A, I, default)
get(A::AbstractArray, I::RangeVecIntList, default) = get!(similar(A, typeof(default), index_shape(I...)), A, I, default)

## structured matrix methods ##
replace_in_print_matrix(A::AbstractMatrix,i::Integer,j::Integer,s::AbstractString) = s
Expand Down Expand Up @@ -1173,7 +1170,7 @@ function cat_t(dims, T::Type, X...)
return _cat(A, shape, catdims, X...)
end

function _cat(A, shape, catdims, X...)
function _cat{N}(A, shape::NTuple{N}, catdims, X...)
N = length(shape)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Looks like it was throwing a warning that I missed.

offsets = zeros(Int, N)
inds = Vector{UnitRange{Int}}(N)
Expand All @@ -1187,7 +1184,8 @@ function _cat(A, shape, catdims, X...)
inds[i] = 1:shape[i]
end
end
A[inds...] = x
I::NTuple{N, UnitRange{Int}} = (inds...,)
A[I...] = x
end
return A
end
Expand Down Expand Up @@ -1759,7 +1757,7 @@ function mapslices(f, A::AbstractArray, dims::AbstractVector)
idx = Any[first(ind) for ind in indices(A)]
itershape = tuple(dimsA[otherdims]...)
for d in dims
idx[d] = Colon()
idx[d] = Slice(indices(A, d))
end

Aslice = A[idx...]
Expand Down
16 changes: 8 additions & 8 deletions base/array.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
typealias AbstractVector{T} AbstractArray{T,1}
typealias AbstractMatrix{T} AbstractArray{T,2}
typealias AbstractVecOrMat{T} Union{AbstractVector{T}, AbstractMatrix{T}}
typealias RangeIndex Union{Int, Range{Int}, AbstractUnitRange{Int}, Colon}
typealias RangeIndex Union{Int, Range{Int}, AbstractUnitRange{Int}}
typealias DimOrInd Union{Integer, AbstractUnitRange}
typealias IntOrInd Union{Int, AbstractUnitRange}
typealias DimsOrInds{N} NTuple{N,DimOrInd}
Expand Down Expand Up @@ -448,8 +448,8 @@ done(a::Array,i) = (@_inline_meta; i == length(a)+1)
## Indexing: getindex ##

# This is more complicated than it needs to be in order to get Win64 through bootstrap
getindex(A::Array, i1::Real) = arrayref(A, to_index(i1))
getindex(A::Array, i1::Real, i2::Real, I::Real...) = (@_inline_meta; arrayref(A, to_index(i1), to_index(i2), to_indexes(I...)...)) # TODO: REMOVE FOR #14770
getindex(A::Array, i1::Int) = arrayref(A, i1)
getindex(A::Array, i1::Int, i2::Int, I::Int...) = (@_inline_meta; arrayref(A, i1, i2, I...)) # TODO: REMOVE FOR #14770

# Faster contiguous indexing using copy! for UnitRange and Colon
function getindex(A::Array, I::UnitRange{Int})
Expand All @@ -472,13 +472,13 @@ function getindex(A::Array, c::Colon)
end

# This is redundant with the abstract fallbacks, but needed for bootstrap
function getindex{S,T<:Real}(A::Array{S}, I::Range{T})
return S[ A[to_index(i)] for i in I ]
function getindex{S}(A::Array{S}, I::Range{Int})
return S[ A[i] for i in I ]
end

## Indexing: setindex! ##
setindex!{T}(A::Array{T}, x, i1::Real) = arrayset(A, convert(T,x)::T, to_index(i1))
setindex!{T}(A::Array{T}, x, i1::Real, i2::Real, I::Real...) = (@_inline_meta; arrayset(A, convert(T,x)::T, to_index(i1), to_index(i2), to_indexes(I...)...)) # TODO: REMOVE FOR #14770
setindex!{T}(A::Array{T}, x, i1::Int) = arrayset(A, convert(T,x)::T, i1)
setindex!{T}(A::Array{T}, x, i1::Int, i2::Int, I::Int...) = (@_inline_meta; arrayset(A, convert(T,x)::T, i1, i2, I...)) # TODO: REMOVE FOR #14770

# These are redundant with the abstract fallbacks but needed for bootstrap
function setindex!(A::Array, x, I::AbstractVector{Int})
Expand Down Expand Up @@ -1313,7 +1313,7 @@ function find(testf::Function, A)
return I
end
_index_remapper(A::AbstractArray) = linearindices(A)
_index_remapper(iter) = Colon() # safe for objects that don't implement length
_index_remapper(iter) = OneTo(typemax(Int)) # safe for objects that don't implement length

"""
find(A)
Expand Down
34 changes: 34 additions & 0 deletions base/deprecated.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,40 @@ eval(Base.Dates, quote
recur{T<:TimeType}(fun::Function, start::T, stop::T; step::Period=Day(1), negate::Bool=false, limit::Int=10000) = recur(fun, start:step:stop; negate=negate)
end)

# Index conversions revamp; #19730
function getindex(A::LogicalIndex, i::Int)
depwarn("getindex(A::LogicalIndex, i) is deprecated; use iteration or index into the result of `collect(A)` instead.", :getindex)
checkbounds(A, i)
first(Iterators.drop(A, i-1))
end
function to_indexes(I...)
depwarn("to_indexes is deprecated; pass both the source array `A` and indices as `to_indices(A, $(I...))` instead.", :to_indexes)
map(_to_index, I)
end
_to_index(i) = to_index(I)
_to_index(c::Colon) = c
const _colon_usage_msg = "convert Colons to a set of indices for indexing into array `A` by passing them in a complete tuple of indices `I` to `to_indices(A, I)`"
function getindex(::Colon, i)
depwarn("getindex(::Colon, i) is deprecated; $_colon_usage_msg", :getindex)
to_index(i)
end
function unsafe_getindex(::Colon, i::Integer)
depwarn("getindex(::Colon, i) is deprecated; $_colon_usage_msg", :unsafe_getindex)
to_index(i)
end
function step(::Colon)
depwarn("step(::Colon) is deprecated; $_colon_usage_msg", :step)
1
end
function isempty(::Colon)
depwarn("isempty(::Colon) is deprecated; $_colon_usage_msg", :isempty)
false
end
function in(::Integer, ::Colon)
depwarn("in(::Integer, ::Colon) is deprecated; $_colon_usage_msg", :in)
true
end

# #18931
@deprecate cummin(A, dim=1) accumulate(min, A, dim)
@deprecate cummax(A, dim=1) accumulate(max, A, dim)
Expand Down
7 changes: 7 additions & 0 deletions base/essentials.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,13 @@ function isassigned(v::SimpleVector, i::Int)
end

# index colon
"""
Colon()

Colons (:) are used to signify indexing entire objects or dimensions at once.
Very few operations are defined on Colons directly; instead they are converted
to `Slice`s upon indexing (within `to_indices`).
"""
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

this should go into the stdlib doc index

immutable Colon
end
const (:) = Colon()
Expand Down
Loading