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

Fixes, tests, & the future of Base.Slice #21052

Closed
wants to merge 1 commit into from
Closed

Fixes, tests, & the future of Base.Slice #21052

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
4 changes: 3 additions & 1 deletion base/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,13 +440,15 @@ 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
checkindex(::Type{Bool}, inds::AbstractUnitRange, s::Slice) =
(first(s) >= first(inds)) & (last(s) <= last(inds))
Copy link
Member

Choose a reason for hiding this comment

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

This is a little unfortunate. I suppose it's not a deal-breaker, but it stinks if we need to check bounds for indexing with colons.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was my main concern too. Colon is effectively an unlimited Slice, which is why we never needed to check bounds before; now that it's finite, we have to check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we resolve this by making Colon a subtype of AbstractUnitRange?

Copy link
Member

Choose a reason for hiding this comment

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

I think that'd put us right back to where we are now, only s/Slice/Colon/.

In order for Colon to be a range, it must know where to start and how long it is. That can only be resolved upon indexing (within to_indices). Thus, there must be some way of constructing a colon with a given start and length. But if a user ends up using that same construction then it might not accurately describe an entire dimension. And now we're right back to where we started, except this time it's a more visible, exported object that users regularly construct.

function checkindex(::Type{Bool}, inds::AbstractUnitRange, r::Range)
@_propagate_inbounds_meta
isempty(r) | (checkindex(Bool, inds, first(r)) & checkindex(Bool, inds, last(r)))
end
checkindex(::Type{Bool}, indx::AbstractUnitRange, I::AbstractVector{Bool}) = indx == indices1(I)
checkindex(::Type{Bool}, indx::AbstractUnitRange, I::AbstractArray{Bool}) = false
checkindex(::Type{Bool}, indx::Slice, I::AbstractVector{Bool}) = indx.indices == indices1(I)
function checkindex(::Type{Bool}, inds::AbstractUnitRange, I::AbstractArray)
@_inline_meta
b = true
Expand Down
53 changes: 47 additions & 6 deletions base/indices.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,20 +218,40 @@ to_indices(A, inds, I::Tuple{Any, Vararg{Any}}) =
_maybetail(::Tuple{}) = ()
_maybetail(t::Tuple) = tail(t)

# TODO: reparametrize in a manner consistent with other AbstractUnitRanges,
# rename (IdempotentRange? IdentityRange?), and move to ranges.jl
"""
Slice(indices)
Slice(r::AbstractUnitRange) -> s

Represent an AbstractUnitRange of indices as a vector of the indices themselves.
Construct an `AbstractUnitRange` where `s[i] == i` for any valid `i`;
equivalently, `indices(s, 1) == r` and `s[s] === s`.

These are particularly useful for creating `view`s of arrays that
preserve the supplied indices:
```jldoctest
julia> a = rand(8);

julia> v1 = view(a, 3:5);

julia> indices(v1, 1)
Base.OneTo(3)

julia> s = Base.Slice(3:5)
Base.Slice(3:5)

julia> v2 = view(a, s);

julia> indices(v2, 1)
3:5
```

Upon calling `to_indices()`, Colons are converted to Slice objects to represent
the indices over which the Colon spans. Slice objects are themselves unit
ranges with the same indices as those they wrap. This means that indexing into
Slice objects with an integer always returns that exact integer, and they
iterate over all the wrapped indices, even supporting offset indices.
the indices over which the Colon spans.
"""
struct Slice{T<:AbstractUnitRange} <: AbstractUnitRange{Int}
indices::T
end
Slice(S::Slice) = S # idempotent
indices(S::Slice) = (S.indices,)
unsafe_indices(S::Slice) = (S.indices,)
indices1(S::Slice) = S.indices
Expand All @@ -242,7 +262,28 @@ size(S::Slice) = first(S.indices) == 1 ? (length(S.indices),) : errmsg(S)
length(S::Slice) = first(S.indices) == 1 ? length(S.indices) : errmsg(S)
unsafe_length(S::Slice) = first(S.indices) == 1 ? unsafe_length(S.indices) : errmsg(S)
getindex(S::Slice, i::Int) = (@_inline_meta; @boundscheck checkbounds(S, i); i)
function getindex(r::Slice, s::AbstractUnitRange{<:Integer})
@_inline_meta
@boundscheck checkbounds(r, s)
s
end
show(io::IO, r::Slice) = print(io, "Base.Slice(", r.indices, ")")
start(S::Slice) = start(S.indices)
next(S::Slice, s) = next(S.indices, s)
done(S::Slice, s) = done(S.indices, s)
intersect{I<:AbstractUnitRange{Int}}(r::Slice{I}, s::Slice{I}) =
Slice(convert(I, max(first(r), first(s)):min(last(r), last(s))))
intersect(r::Slice, s::Slice) =
Slice(max(first(r), first(s)):min(last(r), last(s)))
reverse(r::Slice) = error("reverse is not supported for Base.Slice")
Copy link
Contributor

Choose a reason for hiding this comment

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

is this #7512 or avoiding a fallback?

Copy link
Member

Choose a reason for hiding this comment

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

Currently:

julia> reverse(Base.Slice(-10:10))
10:-1:-10

julia> indices(reverse(Base.Slice(-10:10)))
(Base.OneTo(21),)

Those indices should be the same as the indices of the original Slice object (-10:10).

Copy link
Contributor

Choose a reason for hiding this comment

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

is that Slices problem, or indices' ?

Copy link
Member

@mbauman mbauman Mar 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The general AbstractArray reverse would throw an error for an offset array, but OrdinalRange just always creates a 1-indexed array step range via colon().

Copy link
Member

@mbauman mbauman Mar 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

… and the correct result isn't representable without general offset array support. Slice itself cannot represent reverse(Base.Slice(-10:10)) — which should be an offset array with values 10:-1:-10 and indices -10:10. It could be implemented like I did for length and friends above, where it succeeds for 1-based slices but errors otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

In Base, we can't currently create an array with indices -10:10 unless it also holds values -10:10 (courtesy of Base.Slice). It's a byproduct of being able to create some but not all types of OffsetArrays.

sortperm(r::Slice) = r
==(r::Slice, s::Slice) = (first(r) == first(s)) & (step(r) == step(s)) & (last(r) == last(s))
==(r::Slice, s::OrdinalRange) = (first(r) == first(s) == 1) & (step(r) == step(s)) & (last(r) == last(s))
==(s::OrdinalRange, r::Slice) = r == s
promote_rule{R1,R2}(::Type{Slice{R1}},::Type{Slice{R2}}) =
Slice{promote_type(R1,R2)}
convert{R<:AbstractUnitRange{Int}}(::Type{Slice{R}}, r::Slice{R}) = r
convert{R<:AbstractUnitRange{Int}}(::Type{Slice{R}}, r::Slice) =
Slice(convert(R, r.indices))
convert{R<:AbstractUnitRange{Int}}(::Type{Slice}, r::Slice{R}) =
convert(Slice{R}, r)
6 changes: 5 additions & 1 deletion base/subarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,10 +266,14 @@ end
compute_offset1(parent, stride1::Integer, I::Tuple) =
(@_inline_meta; compute_offset1(parent, stride1, find_extended_dims(I)..., I))
compute_offset1(parent, stride1::Integer, dims::Tuple{Int}, inds::Tuple{Slice}, I::Tuple) =
(@_inline_meta; compute_linindex(parent, I) - stride1*first(indices(parent, dims[1]))) # index-preserving case
(@_inline_meta; compute_linindex(parent, I) - stride1*first(inds[1]))
compute_offset1(parent, stride1::Integer, dims, inds, I::Tuple) =
(@_inline_meta; compute_linindex(parent, I) - stride1) # linear indexing starts with 1

function compute_linindex(parent::AbstractVector, I::Tuple{Any})
@_inline_meta
first(I[1])
end
function compute_linindex{N}(parent, I::NTuple{N,Any})
@_inline_meta
IP = fill_to_length(indices(parent), OneTo(1), Val{N})
Expand Down
100 changes: 100 additions & 0 deletions test/arrayops.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2071,3 +2071,103 @@ end
Base.:*(a::T11053, b::Real) = T11053(a.a*b)
Base.:(==)(a::T11053, b::T11053) = a.a == b.a
@test [T11053(1)] * 5 == [T11053(1)] .* 5 == [T11053(5.0)]

@testset "Slice" begin
r = Base.Slice(0:-5)
@test isempty(r)
r = Base.Slice(0:2)
@test Base.Slice(r) === r
@test !isempty(r)
@test indices(r) === (0:2,)
@test step(r) == 1
@test first(r) == 0
@test last(r) == 2
@test minimum(r) == 0
@test maximum(r) == 2
@test r[0] == 0
@test r[1] == 1
@test r[2] == 2
@test r != 0:2
@test r == Base.Slice(0:2)
@test r === Base.Slice(0:2)
@test_throws BoundsError r[3]
@test_throws BoundsError r[-1]
@test r[0:2] === 0:2
@test r[r] === r
@test r[Base.Slice(1:2)] === Base.Slice(1:2)
@test r[1:2] === 1:2
@test_throws BoundsError r[Base.Slice(1:3)]
@test_throws BoundsError r[1:3]
k = -1
for i in r
@test i == (k+=1)
end
@test k == 2
@test intersect(r, Base.Slice(-1:1)) === intersect(Base.Slice(-1:1), r) === Base.Slice(0:1)
@test intersect(r, -1:5) === intersect(-1:5, r) === 0:2
@test intersect(r, 2:5) === intersect(2:5, r) === 2:2
# Not ideal, but at least this isn't wrong...
@test_throws ErrorException r+1
@test_throws ErrorException r-1
@test_throws ErrorException 2*r
@test_throws ErrorException r/2
@test_throws ErrorException reverse(r)

r = Base.Slice(2:4)
@test r != 2:4
@test r == Base.Slice(2:4)
@test r === Base.Slice(2:4)
@test Base.Slice(1:4) == 1:4
@test checkindex(Bool, r, 4)
@test !checkindex(Bool, r, 5)
@test checkindex(Bool, r, :)
@test checkindex(Bool, r, 2:4)
@test !checkindex(Bool, r, 1:5)
@test !checkindex(Bool, r, trues(4))
@test !checkindex(Bool, r, trues(3))
@test checkindex(Bool, r, view(trues(5), r))
@test !in(1, r)
@test in(2, r)
@test in(4, r)
@test !in(5, r)
@test issorted(r)
@test maximum(r) == 4
@test minimum(r) == 2
@test sortperm(r) == r

r = Base.Slice(Int16(0):Int16(4))
@test start(r) === 0
k = -1
for i in r
@test i == (k+=1)
end
@test k == 4
x, y = promote(r, Base.Slice(2:4))
@test x === Base.Slice(0:4)
@test y === Base.Slice(2:4)
x, y = promote(Base.Slice(4:5), 0:7)
@test x === 4:5
@test y === 0:7
r = Base.Slice(Int128(1):Int128(10))
@test length(r) === Int128(10)
end

@testset "Slice with view" begin
a = rand(8)
idr = Base.Slice(2:4)
v = view(a, idr)
@test indices(v) == (2:4,)
@test v[2] == a[2]
@test v[3] == a[3]
@test v[4] == a[4]
@test_throws BoundsError v[1]
@test_throws BoundsError v[5]

a = rand(5, 5)
idr2 = Base.Slice(3:4)
v = view(a, idr, idr2)
@test indices(v) == (2:4, 3:4)
@test v[2,3] == a[2,3]
end

nothing
7 changes: 7 additions & 0 deletions test/offsetarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ for i = 1:9 @test A_3_3[i] == i end
@test_throws BoundsError A[[true true; false true]]

# view
x = OffsetArray(1:8, (-2,))
S = view(x, :)
@test indices(S) == (-1:6,)
@test S[-1] == 1
@test S[6] == 8
@test_throws BoundsError S[-2]
@test_throws BoundsError S[7]
S = view(A, :, 3)
@test S == OffsetArray([1,2], (A.offsets[1],))
@test S[0] == 1
Expand Down