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

Iteration failure with CartesianIndex #17

Closed
timholy opened this issue May 25, 2019 · 3 comments
Closed

Iteration failure with CartesianIndex #17

timholy opened this issue May 25, 2019 · 3 comments

Comments

@timholy
Copy link
Collaborator

timholy commented May 25, 2019

julia> A = rand(5,5,5);

julia> S = Slices(A, True(), False(), False());

julia> size(S)
(5, 5)

julia> for s in S
           @show size(s)
       end
ERROR: ArgumentError: tuple must be non-empty
Stacktrace:
 [1] first(::Tuple{}) at ./tuple.jl:77
 [2] setindex(::Tuple{Base.OneTo{Int64}}, ::Tuple{}, ::Tuple{True}) at /home/tim/.julia/packages/JuliennedArrays/t66eT/src/JuliennedArrays.jl:39
 [3] setindex(::Tuple{Base.OneTo{Int64},Base.OneTo{Int64}}, ::Tuple{CartesianIndex{2}}, ::Tuple{True,True}) at /home/tim/.julia/packages/JuliennedArrays/t66eT/src/JuliennedArrays.jl:43 (repeats 2 times)
 [4] getindex at /home/tim/.julia/packages/JuliennedArrays/t66eT/src/JuliennedArrays.jl:58 [inlined]
 [5] iterate at ./abstractarray.jl:860 [inlined]
 [6] iterate(::Slices{SubArray{Float64,1,Array{Float64,3},Tuple{Base.OneTo{Int64},Int64,Int64},true},2,Array{Float64,3},Tuple{True,False,False}}) at ./abstractarray.jl:858
 [7] top-level scope at ./none:0

These kinds of things could be caught in real tests 😉.

@bramtayl
Copy link
Owner

Hmm, ok, is the solution just to define:

@propagate_inbounds getindex(it::Slices, index::CartesianIndex) = 
    getindex(it, index.I...)

?

If so, should there be a note in here about supporting ::CartesianIndex?

@bramtayl
Copy link
Owner

Never mind, index::Int... seems to do the trick. Fixed on master.

@johnnychen94
Copy link
Contributor

Never mind, index::Int... seems to do the trick. Fixed on master.

This would incorrectly capture Linear Indexing getindex(S, 1) and some weird indexinggetindex(S, 1, 2, 3, 4, 5). Because Align/Slices internally uses the cartesian index to calculate where to insert the dimensions, we need to use the fallback version of getindex for linear indexing cases.

Patch submitted in #28.

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

3 participants