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/WIP: fully deprecate partial indexing. Fixes #14770. #20600

Closed
wants to merge 1 commit into from
Closed

RFC/WIP: fully deprecate partial indexing. Fixes #14770. #20600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
64 changes: 34 additions & 30 deletions base/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ unsafe_indices(r::Range) = (OneTo(unsafe_length(r)),) # Ranges use checked_sub f
"""
linearindices(A)

Returns a `UnitRange` specifying the valid range of indices for `A[i]`
Returns a `AbstractUnitRange` specifying the valid range of indices for `A[i]`
where `i` is an `Int`. For arrays with conventional indexing (indices
start at 1), or any multidimensional array, this is `1:length(A)`;
however, for one-dimensional arrays with unconventional indices, this
Expand All @@ -89,6 +89,9 @@ julia> extrema(b)
"""
linearindices(A) = (@_inline_meta; OneTo(_length(A)))
linearindices(A::AbstractVector) = (@_inline_meta; indices1(A))
linearindices(inds::Tuple{Vararg{AbstractUnitRange}}) =
(@_inline_meta; OneTo(prod(map(unsafe_length, inds))))

eltype(::Type{<:AbstractArray{E}}) where {E} = E
elsize{T}(::AbstractArray{T}) = sizeof(T)

Expand Down Expand Up @@ -363,21 +366,7 @@ function checkbounds_indices(::Type{Bool}, IA::Tuple{Any}, I::Tuple{Any})
end
function checkbounds_indices(::Type{Bool}, IA::Tuple, I::Tuple{Any})
@_inline_meta
checkbounds_linear_indices(Bool, IA, I[1])
end
function checkbounds_linear_indices(::Type{Bool}, IA::Tuple, i)
@_inline_meta
if checkindex(Bool, IA[1], i)
return true
elseif checkindex(Bool, OneTo(trailingsize(IA)), i) # partial linear indexing
partial_linear_indexing_warning_lookup(length(IA))
return true # TODO: Return false after the above function is removed in deprecated.jl
end
return false
end
function checkbounds_linear_indices(::Type{Bool}, IA::Tuple, i::Union{Slice,Colon})
partial_linear_indexing_warning_lookup(length(IA))
true
checkbounds_indices(Bool, (linearindices(IA),), I)
end
checkbounds_indices(::Type{Bool}, ::Tuple, ::Tuple{}) = true

Expand Down Expand Up @@ -824,18 +813,32 @@ _getindex(::LinearIndexing, A::AbstractArray, I...) = error("indexing $(typeof(A

## LinearFast Scalar indexing: canonical method is one Int
_getindex(::LinearFast, A::AbstractArray, i::Int) = (@_propagate_inbounds_meta; getindex(A, i))
_getindex(::LinearFast, A::AbstractArray) = (@_propagate_inbounds_meta; getindex(A, _to_linear_index(A)))
_getindex(::LinearFast, A::AbstractArray) = (@_propagate_inbounds_meta; getindex(A, 1))
function _getindex(::LinearFast, A::AbstractArray, I::Int...)
@_inline_meta
@boundscheck checkbounds(A, I...) # generally _to_linear_index requires bounds checking
@inbounds r = getindex(A, _to_linear_index(A, I...))
r
end
_to_linear_index(A::AbstractVector) = (@_inline_meta; first(indices1(A)))
_to_linear_index(A::AbstractArray) = 1
_to_linear_index(A::AbstractArray, i::Int) = i
_to_linear_index(A::AbstractVector, i::Int, I::Int...) = i # TODO: DEPRECATE FOR #14770
_to_linear_index{T,N}(A::AbstractArray{T,N}, I::Vararg{Int,N}) = (@_inline_meta; sub2ind(A, I...))
_to_linear_index(A::AbstractArray) = 1 # TODO: DEPRECATE FOR #14770
_to_linear_index(A::AbstractArray, I::Int...) = (@_inline_meta; sub2ind(A, I...)) # TODO: DEPRECATE FOR #14770
function _to_linear_index{T,N}(A::AbstractArray{T,N}, I::Int...)
@_inline_meta
J, Jrem = IteratorsMD.split(I, Val{N})
_to_linear_index(A, J, Jrem)
end
# It's safe to drop Jrem, because we've already bounds-checked
_to_linear_index(A::AbstractVector, J::Tuple, Jrem::Tuple) = (@_inline_meta; J[1])
_to_linear_index(A::AbstractArray, J::Tuple, Jrem::Tuple) = (@_inline_meta; sub2ind(A, J...))
# TODO: uncomment when deprecation period for partial linear indexing has ended
# function _to_linear_index(A::AbstractVector, J::Tuple, Jrem::Tuple{})
# throw(ArgumentError("partial linear indexing is not allowed. Use `reshape(A, Val{$(length(J))})` to make the dimensionality of the array match the number of indices"))
# end
# function _to_linear_index(A::AbstractArray, J::Tuple, Jrem::Tuple{})
# throw(ArgumentError("partial linear indexing is not allowed. Use `reshape(A, Val{$(length(J))})` to make the dimensionality of the array match the number of indices"))
# end

## LinearSlow Scalar indexing: Canonical method is full dimensionality of Ints
_getindex(::LinearSlow, A::AbstractArray) = (@_propagate_inbounds_meta; getindex(A, _to_subscript_indices(A)...))
Expand All @@ -847,16 +850,18 @@ function _getindex(::LinearSlow, A::AbstractArray, I::Int...)
end
_getindex{T,N}(::LinearSlow, A::AbstractArray{T,N}, I::Vararg{Int, N}) = (@_propagate_inbounds_meta; getindex(A, I...))
_to_subscript_indices(A::AbstractArray, i::Int) = (@_inline_meta; _unsafe_ind2sub(A, i))
_to_subscript_indices{T,N}(A::AbstractArray{T,N}) = (@_inline_meta; fill_to_length((), 1, Val{N})) # TODO: DEPRECATE FOR #14770
_to_subscript_indices{T}(A::AbstractArray{T,0}) = () # TODO: REMOVE FOR #14770
_to_subscript_indices{T}(A::AbstractArray{T,0}, i::Int) = () # TODO: REMOVE FOR #14770
_to_subscript_indices{T}(A::AbstractArray{T,0}, I::Int...) = () # TODO: DEPRECATE FOR #14770
function _to_subscript_indices{T,N}(A::AbstractArray{T,N}, I::Int...) # TODO: DEPRECATE FOR #14770
_to_subscript_indices{T,N}(A::AbstractArray{T,N}) = (@_inline_meta; map(first, indices(A)))
_to_subscript_indices{T}(A::AbstractArray{T,0}, I::Int...) = ()
function _to_subscript_indices{T,N}(A::AbstractArray{T,N}, I::Int...)
@_inline_meta
J, _ = IteratorsMD.split(I, Val{N}) # (maybe) drop any trailing indices
sz = _remaining_size(J, size(A)) # compute trailing size (overlapping the final index)
(front(J)..., _unsafe_ind2sub(sz, last(J))...) # (maybe) extend the last index
end
J, Jrem = IteratorsMD.split(I, Val{N})
_to_subscript_indices(A, J, Jrem)
end
_to_subscript_indices(A, J::Tuple, Jrem::Tuple) = J # already bounds-checked, safe to drop Jrem
# TODO: uncomment when deprecation period for partial linear indexing has ended
# function _to_subscript_indices(A::AbstractArray, J::Tuple, Jrem::Tuple{})
# throw(ArgumentError("partial linear indexing is not allowed. Use `reshape(A, Val{$(length(J))})` to make the dimensionality of the array match the number of indices"))
# end
_to_subscript_indices{T,N}(A::AbstractArray{T,N}, I::Vararg{Int,N}) = I
_remaining_size(::Tuple{Any}, t::Tuple) = t
_remaining_size(h::Tuple, t::Tuple) = (@_inline_meta; _remaining_size(tail(h), tail(t)))
Expand Down Expand Up @@ -926,7 +931,6 @@ end

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)
fill!(X, default)
dst, src = indcopy(size(A), I)
Expand Down
7 changes: 5 additions & 2 deletions base/array.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,8 +471,9 @@ 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{T}(A::Array{T,0}) = arrayref(A, 1)
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
getindex{T,N}(A::Array{T,N}, I::Vararg{Int,N}) = (@_inline_meta; arrayref(A, I...))

# Faster contiguous indexing using copy! for UnitRange and Colon
function getindex(A::Array, I::UnitRange{Int})
Expand Down Expand Up @@ -500,8 +501,9 @@ function getindex{S}(A::Array{S}, I::Range{Int})
end

## Indexing: setindex! ##
setindex!{T}(A::Array{T,0}, x) = arrayset(A, convert(T,x)::T, 1)
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
setindex!{T,N}(A::Array{T,N}, x, I::Vararg{Int,N}) = (@_inline_meta; arrayset(A, convert(T,x)::T, I...))

# These are redundant with the abstract fallbacks but needed for bootstrap
function setindex!(A::Array, x, I::AbstractVector{Int})
Expand Down Expand Up @@ -548,6 +550,7 @@ function setindex!{T}(A::Array{T}, X::Array{T}, c::Colon)
end

setindex!(A::Array, x::Number, ::Colon) = fill!(A, x)
setindex!{T}(A::Array{T,0}, x::Number) = fill!(A, x) # ambiguity resolution
setindex!{T, N}(A::Array{T, N}, x::Number, ::Vararg{Colon, N}) = fill!(A, x)

# efficiently grow an array
Expand Down
50 changes: 14 additions & 36 deletions base/deprecated.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,43 +1032,19 @@ isempty(::Task) = error("isempty not defined for Tasks")
end

# Deprecate partial linear indexing
function partial_linear_indexing_warning_lookup(nidxs_remaining)
# We need to figure out how many indices were passed for a sensible deprecation warning
opts = JLOptions()
if opts.depwarn > 0
# Find the caller -- this is very expensive so we don't want to do it twice
bt = backtrace()
found = false
call = StackTraces.UNKNOWN
caller = StackTraces.UNKNOWN
for frame in bt
lkups = StackTraces.lookup(frame)
for caller in lkups
if caller === StackTraces.UNKNOWN
continue
end
found && @goto found
if caller.func in (:getindex, :setindex!, :view)
found = true
call = caller
end
end
end
@label found
fn = "`reshape`"
if call != StackTraces.UNKNOWN && !isnull(call.linfo)
# Try to grab the number of dimensions in the parent array
mi = get(call.linfo)
args = mi.specTypes.parameters
if length(args) >= 2 && args[2] <: AbstractArray
fn = "`reshape(A, Val{$(ndims(args[2]) - nidxs_remaining + 1)})`"
end
end
_depwarn("Partial linear indexing is deprecated. Use $fn to make the dimensionality of the array match the number of indices.", opts, bt, caller)
end
check_oneto(A::AbstractArray) = check_oneto(indices(A))
check_oneto(inds::Tuple{Vararg{OneTo}}) = nothing
check_oneto(inds::Tuple{AbstractUnitRange,Vararg{AbstractUnitRange}}) = error("partial linear indexing is not supported for arrays with non-1 indexing")
function _to_linear_index(A::AbstractArray, J::Tuple, Jrem::Tuple{})
check_oneto(A)
depwarn("partial linear indexing is deprecated. Use `reshape(A, Val{$(length(J))})` to make the dimensionality of the array match the number of indices", :_to_linear_index)
sub2ind(A, J...)
end
function partial_linear_indexing_warning(n)
depwarn("Partial linear indexing is deprecated. Use `reshape(A, Val{$n})` to make the dimensionality of the array match the number of indices.", (:getindex, :setindex!, :view))
function _to_subscript_indices(A::AbstractArray, J::Tuple, Jrem::Tuple{})
check_oneto(A)
depwarn("partial linear indexing is deprecated. Use `reshape(A, Val{$(length(J))})` to make the dimensionality of the array match the number of indices", :_to_subscript_indices)
sz = _remaining_size(J, indices(A)) # compute trailing size (overlapping the final index)
(front(J)..., _unsafe_ind2sub(sz, last(J))...)
end

# Deprecate Array(T, dims...) in favor of proper type constructors
Expand Down Expand Up @@ -1262,6 +1238,8 @@ for f in (:airyai, :airyaiprime, :airybi, :airybiprime, :airyaix, :airyaiprimex,
end

# END 0.6 deprecations
# IMPORTANT: when the 0.6 deprecations are removed, uncomment error messages for
# _to_linear_index and _to_subscript_indices in abstractarray.jl

# BEGIN 1.0 deprecations
# END 1.0 deprecations
2 changes: 2 additions & 0 deletions base/multidimensional.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,10 @@ end
# Indexing into Array with mixtures of Integers and CartesianIndices is
# extremely performance-sensitive. While the abstract fallbacks support this,
# codegen has extra support for SIMDification that sub2ind doesn't (yet) support
@propagate_inbounds getindex(A::Array, I::Integer...) = _getindex(LinearFast(), A, to_indices(A, I)...)
@propagate_inbounds getindex(A::Array, i1::Union{Integer, CartesianIndex}, I::Union{Integer, CartesianIndex}...) =
A[to_indices(A, (i1, I...))...]
@propagate_inbounds setindex!(A::Array, v, I::Integer...) = _setindex!(LinearFast(), A, v, to_indices(A, I)...)
@propagate_inbounds setindex!(A::Array, v, i1::Union{Integer, CartesianIndex}, I::Union{Integer, CartesianIndex}...) =
(A[to_indices(A, (i1, I...))...] = v; A)

Expand Down
83 changes: 64 additions & 19 deletions test/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ Base.setindex!{T}(A::TSlow{T,5}, v, i1::Int, i2::Int, i3::Int, i4::Int, i5::Int)
(A.data[(i1,i2,i3,i4,i5)] = v)

const can_inline = Base.JLOptions().can_inline != 0
const depwarns_not_error = Base.JLOptions().depwarn < 2
function test_scalar_indexing{T}(::Type{T}, shape, ::Type{TestAbstractArray})
N = prod(shape)
A = reshape(collect(1:N), shape)
Expand All @@ -263,27 +264,44 @@ function test_scalar_indexing{T}(::Type{T}, shape, ::Type{TestAbstractArray})
end
end
# Test linear indexing and partial linear indexing
@test A == B
i=0
for i1 = 1:length(B)
i += 1
@test A[i1] == B[i1] == i
end
i=0
for i2 = 1:size(B, 2)
for i1 = 1:size(B, 1)
i += 1
@test A[i1,i2] == B[i1,i2] == i
# TODO (#14770): run only for ndims(A) <= 2, elim. redirect_stderr stuff
if ndims(A) <= 2 || depwarns_not_error
filename = tempname()
open(filename, "w") do f
redirect_stderr(f) do
Copy link
Contributor

Choose a reason for hiding this comment

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

would @test_warn work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the warning only gets issued on the first call, it would make this sensitive to which tests get assigned to which workers.

i=0
for i2 = 1:size(B, 2)
for i1 = 1:size(B, 1)
i += 1
@test A[i1,i2] == B[i1,i2] == i
end
end
end
end
rm(filename)
end
@test A == B
i=0
for i3 = 1:size(B, 3)
for i2 = 1:size(B, 2)
for i1 = 1:size(B, 1)
i += 1
@test A[i1,i2,i3] == B[i1,i2,i3] == i
if ndims(A) <= 3 || depwarns_not_error
filename = tempname()
open(filename, "w") do f
redirect_stderr(f) do
i=0
for i3 = 1:size(B, 3)
for i2 = 1:size(B, 2)
for i1 = 1:size(B, 1)
i += 1
@test A[i1,i2,i3] == B[i1,i2,i3] == i
end
end
end
end
end
rm(filename)
end
# Test zero-dimensional accesses
@test A[] == B[] == A[1] == B[1] == 1
Expand Down Expand Up @@ -369,22 +387,49 @@ function test_vector_indexing{T}(::Type{T}, shape, ::Type{TestAbstractArray})
# Test adding dimensions with matrices
idx1 = rand(1:size(A, 1), 3)
idx2 = rand(1:size(A, 2), 4, 5)
@test B[idx1, idx2] == A[idx1, idx2] == reshape(A[idx1, vec(idx2)], 3, 4, 5) == reshape(B[idx1, vec(idx2)], 3, 4, 5)
@test B[1, idx2] == A[1, idx2] == reshape(A[1, vec(idx2)], 4, 5) == reshape(B[1, vec(idx2)], 4, 5)
# #14770
if ndims(A) <= 2 || depwarns_not_error
filename = tempname()
open(filename, "w") do f
redirect_stderr(f) do
@test B[idx1, idx2] == A[idx1, idx2] == reshape(A[idx1, vec(idx2)], 3, 4, 5) == reshape(B[idx1, vec(idx2)], 3, 4, 5)
@test B[1, idx2] == A[1, idx2] == reshape(A[1, vec(idx2)], 4, 5) == reshape(B[1, vec(idx2)], 4, 5)
end
end
rm(filename)
end

# test removing dimensions with 0-d arrays
idx0 = reshape([rand(1:size(A, 1))])
@test B[idx0, idx2] == A[idx0, idx2] == reshape(A[idx0[], vec(idx2)], 4, 5) == reshape(B[idx0[], vec(idx2)], 4, 5)
# @test B[reshape([end]), reshape([end])] == A[reshape([end]), reshape([end])] == reshape([A[end,end]]) == reshape([B[end,end]]) # TODO: Re-enable after partial linear indexing deprecation
# TODO (#14770): run only for ndims(A) <= 2, elim. redirect_stderr stuff
if ndims(A) <= 2 || depwarns_not_error
filename = tempname()
open(filename, "w") do f
redirect_stderr(f) do
@test B[idx0, idx2] == A[idx0, idx2] == reshape(A[idx0[], vec(idx2)], 4, 5) == reshape(B[idx0[], vec(idx2)], 4, 5)
# @test B[reshape([end]), reshape([end])] == A[reshape([end]), reshape([end])] == reshape([A[end,end]]) == reshape([B[end,end]]) # TODO: Re-enable after partial linear indexing deprecation
end
end
rm(filename)
end

# test logical indexing
mask = bitrand(shape)
@test B[mask] == A[mask] == B[find(mask)] == A[find(mask)] == find(mask)
@test B[vec(mask)] == A[vec(mask)] == find(mask)
mask1 = bitrand(size(A, 1))
mask2 = bitrand(size(A, 2))
@test B[mask1, mask2] == A[mask1, mask2] == B[find(mask1), find(mask2)]
@test B[mask1, 1] == A[mask1, 1] == find(mask1)
mask2 = bitrand(prod(Base.tail(size(A))))
# TODO (#14770): run only for ndims(A) <= 2, elim. redirect_stderr stuff
if ndims(A) <= 2 || depwarns_not_error
filename = tempname()
open(filename, "w") do f
redirect_stderr(f) do
@test B[mask1, mask2] == A[mask1, mask2] == B[find(mask1), find(mask2)]
@test B[mask1, 1] == A[mask1, 1] == find(mask1)
end
end
rm(filename)
end
end

function test_primitives{T}(::Type{T}, shape, ::Type{TestAbstractArray})
Expand Down
25 changes: 18 additions & 7 deletions test/subarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,17 +126,26 @@ function test_linear(A::ANY, B::ANY)
end

# "mixed" means 2 indexes even for N-dimensional arrays
# TODO (#14770): eliminate this test when deprecations removed
const depwarns_not_error = Base.JLOptions().depwarn < 2
test_mixed{T}(::AbstractArray{T,1}, ::Array) = nothing
test_mixed{T}(::AbstractArray{T,2}, ::Array) = nothing
test_mixed(A, B::Array) = _test_mixed(A, reshape(B, size(A)))
function _test_mixed(A::ANY, B::ANY)
m = size(A, 1)
n = size(A, 2)
isgood = true
for j = 1:n, i = 1:m
if A[i,j] != B[i,j]
isgood = false
break
if depwarns_not_error
filename = tempname()
open(filename, "w") do f
redirect_stderr(f) do
for j = 1:n, i = 1:m
if A[i,j] != B[i,j]
isgood = false
break
end
end
end
end
end
if !isgood
Expand Down Expand Up @@ -229,9 +238,11 @@ function runviews(SB::AbstractArray, indexN, indexNN, indexNNN)
ndims(SB) > 3 && i3 isa Colon && continue # TODO: Re-enable once Colon no longer spans partial trailing dimensions
runsubarraytests(SB, i1, i2, i3)
end
for i2 in indexN, i1 in indexN
i2 isa Colon && continue # TODO: Re-enable once Colon no longer spans partial trailing dimensions
runsubarraytests(SB, i1, i2)
if depwarns_not_error
for i2 in indexN, i1 in indexN
i2 isa Colon && continue # TODO: Re-enable once Colon no longer spans partial trailing dimensions
runsubarraytests(SB, i1, i2)
end
end
for i1 in indexNNN
runsubarraytests(SB, i1)
Expand Down