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.

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

indexing with Begin and End as types #38296

Closed
rafaqz opened this issue Nov 4, 2020 · 6 comments
Closed

indexing with Begin and End as types #38296

rafaqz opened this issue Nov 4, 2020 · 6 comments

Comments

@rafaqz
Copy link
Contributor

rafaqz commented Nov 4, 2020

This syntax always concerned me - it's too magic.

A[4:end]

The magic also causes issues with packages like DimensionalData.jl and presumably others similar to it, where you don't have to index in the right order or include all the axes, or where you use different brackets for special indexing. Ranges with begin and end in them also can't be passed around though other methods.

This alternative uses the singletons Begin and End and a LazyRange object, which becomes a real range in to_indices:

import Base: :, to_indices, to_index, show, tail

abstract type LazyIndex end
struct Begin <: LazyIndex end
struct End <: LazyIndex end

struct LazyRange{T1,T2,T3}
    start::T1
    step::T2
    stop::T3
end

show(io::IO, lr::LazyRange) = println(io, lr.start, ":", lr.step, ":", lr.stop)
show(io::IO, lr::LazyRange{<:Any, Nothing,<:Any}) = println(io, lr.start, ":", lr.stop)

(:)(lz::Type{<:LazyIndex}, stop::Int) = LazyRange(lz, nothing, stop)
(:)(lz::Type{<:LazyIndex}, step::Int, stop::Int) = LazyRange(lz, step, stop)
(:)(start, lz::Type{<:LazyIndex}) = LazyRange(start, nothing, lz)
(:)(start, step, lz::Type{<:LazyIndex}) = LazyRange(start, step, lz)
(:)(lz1::Type{<:LazyIndex}, lz2::Type{<:LazyIndex}) = LazyRange(lz1, nothing, lz2)
(:)(lz1::Type{<:LazyIndex}, step::Int, lz2::Type{<:LazyIndex}) = LazyRange(lz1, step, lz2)

function to_indices(A, axes, I::Tuple{LazyRange,Vararg})
    i = _lazyindices(axes[1], I[1].start):I[1].step:_lazyindices(axes[1], I[1].stop)
    (i, to_indices(A, tail(axes), tail(I))...)
end
function to_indices(A, axes, I::Tuple{LazyRange{<:Any,Nothing,<:Any},Vararg})
    i = _lazyindices(axes[1],  I[1].start):_lazyindices(axes[1],  I[1].stop)
    (i, to_indices(A, tail(axes), tail(I))...)
end
function to_indices(A, axes, I::Tuple{Type,Vararg})
    i = _to_index(A, axes[1], I[1])
    (i, to_indices(A, tail(axes), tail(I))...)
end

_to_index(A, axis, lz::Type{<:LazyIndex}) = _lazyindices(axis, lz)
# Fallback so this is not a breaking change for Type indices
_to_index(A, axis, T::Type) = to_index(A, T)

_lazyindices(axis, ::Type{Begin}) = firstindex(axis)
_lazyindices(axis, ::Type{End}) = lastindex(axis)
_lazyindices(axis, i::Int) = i


@assert [1,2,3][2:End] == [2,3]
@assert [1,2,3][Begin:End] == [1,2,3]
@assert [1,2,3][End:-1:Begin] == [3,2,1]
@assert [1,2,3][Begin:2] == [1,2]
@assert (1:50)[Begin:10:30] == [1,11,21]
@assert (1:50)[20:10:End] == [20,30,40,50]
@assert (1:50)[Begin] == 1
@assert (1:50)[End] == 50
@assert [1 2; 3 4][Begin, Begin] == 1
@assert [1 2; 3 4][End, End] == 4

Ae there any foreseeable problems with this approach?

@yuyichao
Copy link
Contributor

yuyichao commented Nov 4, 2020

Ae there any foreseeable problems with this approach?

Yes, a[div(End, 2)] and all the other functions you can apply on an integer

@ghost
Copy link

ghost commented Nov 4, 2020

the word lazy was very exshaustingakgaga

@timholy
Copy link
Member

timholy commented Nov 4, 2020

Agreed with the limitation @yuyichao pointed out, but you can implement some support. What you're proposing already exists in EndpointRanges.jl.

@rafaqz
Copy link
Contributor Author

rafaqz commented Nov 4, 2020

@yuyichao ok, I see how doing it with the parser has some benefits.

@timholy thanks for the link. I'll point users that want this functionality to that package for now. How you define a few base methods for iend to apply "lazily" (that one was for you @bahasalien) seems to take care of a lot of the use cases, but feels a bit like a hack - so I agree that's still a limitation.

Still end is kind of unsatisfying. People don't seem to understand that it's parser syntax and try using it in odd ways pretty often. But it's probably my fault for hacking getindex syntax for other things.

@mcabbott
Copy link
Contributor

mcabbott commented Nov 4, 2020

See also #35681 for A[::CartesianIndex, end] going wrong.

@mbauman
Copy link
Member

mbauman commented Nov 4, 2020

Personally, I find the lazy Begin/End far more complicated and magical than a simple parser transform. I agree it's unsatisfying and limiting, but it is simple and straightforward. See it with Meta.@lower.

I think this can safely be closed as a dup of #35681.

@mbauman mbauman closed this as completed Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants