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

CartesianIndex cannot iterate/splat #21148

Closed
andyferris opened this issue Mar 24, 2017 · 7 comments
Closed

CartesianIndex cannot iterate/splat #21148

andyferris opened this issue Mar 24, 2017 · 7 comments
Labels
arrays [a, r, r, a, y, s]

Comments

@andyferris
Copy link
Member

It seems unusual to me that the indexable CartesianIndex cannot also iterate (or splat).

julia> start(Base.CartesianIndex(1))
ERROR: MethodError: no method matching start(::CartesianIndex{1})
Closest candidates are:
  start(::SimpleVector) at essentials.jl:259
  start(::Base.MethodList) at reflection.jl:561
  start(::ExponentialBackOff) at error.jl:107

For context, I had tried this for creating an expression in a generated function by splatting out the Cartesian indices (so run-time performance of iteration vs. indexing isn't an issue). The (or rather, my) workaround is significantly less elegant.

@timholy
Copy link
Sponsor Member

timholy commented Mar 24, 2017

The absence is deliberate, since splatting usually has a huge performance penalty without @inline. If you use the generic array indexing machinery (aka, for anything that's a subtype of AbstractArray) you may not need to splat: a[i::CartesianIndex, 3, j::CartesianIndex] works just fine. That said, if you need to splat in a generated function, I think $(i.I...) should work.

Of course, this is something that could be reconsidered.

@andyferris
Copy link
Member Author

i.I should be sufficient, thanks.

The lack of iteration just seemed unusual, is all.

@timholy
Copy link
Sponsor Member

timholy commented Mar 24, 2017

I agree. If splatting weren't a major performance trap for the unwary, I'd have no reservations in saying that we should support splatting.

@JeffBezanson JeffBezanson added the arrays [a, r, r, a, y, s] label Aug 24, 2017
@rapus95
Copy link
Contributor

rapus95 commented Jul 5, 2018

nL = [400, 200]; ly = nL[2]-1
obs = [100, 100]; obsRadius = 20
map((d,x,y)->if d==2; 0 else 0.04*(1.0+1e-4*sin(y/ly*2*pi)) end, CartesianRange(tuple(2, nL...)))

That one can't work in 0.6 since there is no automatic splatting. Though, since that behaviour changes in 0.7 (automatic splatting into arguments) I've been curious wether it could be reconsidered to allow in such case that it's splatted into the arguments of the anonymous function.
because in fact, since map constructs an array i guess that's a valid julian way to create one. (since there is no fill method that accepts a function)

@martinholters
Copy link
Member

BTW, you can also do Tuple(i) instead of i.I, if the latter feels too much like working with internals.

Some thoughts regarding the performance of splatting CartesianIndexes: Non-vararg Tuples are treated specially because their type gives direct information about the number of their elements. Splatting anything else will require inference to figure out whether the length is fixed. That works in simple cases where the iteration state is encapsulated in its type, but for splatting a CartesianIndex, the state is an integer (just like for a Tuple).

So better constant propagation could help. That would require keeping track of partially const tuples: iterate returns a Tuple of value (generally non-constant) and state (often constant for constant input state).

Another approach I've been pondering is to parse f(x..., y...) as Core._apply(f, Tuple(x), Tuple(y)) instead of Core._apply(f, x, y) (and limit the types accepted by _apply to Tuples). That would allow types to provide their own, efficient Tuple constructors. However, I'm not sure how hard it is to come up with a general Tuple(itr) fallback constructor that is as efficient as the current splatting.

Alternatively, one could introduce a new function, say splat(x) = x, and lower to Core._apply(f, splat(x), splat(y)). Then types with efficient conversion to Tuple could add a custom method to splat and enjoy the benefits, while nothing changes otherwise. (This is also completely non-breaking.) Of course, this (and also the convert-to-Tuple variant above) would have potential for abuse by letting splatting behave completely different from ordinary iteration. Don't know how bad that would be.

Or we change the iteration protocol slightly to first do itr´ = iterator_for(itr) (with the fallback iterator_for(itr) = itr) and the operate on itr´. This would provide the same benefits as the splat function but would ensure that splatting and iteration stay coupled.

Nothing of the above strikes me as the obvious way forward.

@mbauman
Copy link
Sponsor Member

mbauman commented Jul 9, 2018

For what it's worth:

I agree. If splatting weren't a major performance trap for the unwary, I'd have no reservations in saying that we should support splatting.

While I initially agreed with this sentiment, I have since changed my view on this. A CartesianIndex is both a representation of one location as well as a handful of indices. In its primary use — as an argument to getindex — it is definitively one thing and not a collection. Now that we're aligning broadcasting and iteration, I think it's better to keep iteration undefined. Were we to treat it as an iterable container, I'd be very surprised in its behavior in broadcasted getindex.

@mbauman
Copy link
Sponsor Member

mbauman commented Dec 4, 2018

In its primary use ... it is definitively one thing and not a collection

We've since doubled down on this view. I think we can close this issue as resolved/won't change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

No branches or pull requests

6 participants