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

Make CartesianRange an AbstractArray and deprecate sub2ind and ind2sub #25113

Merged
merged 3 commits into from
Dec 16, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Dec 15, 2017

A squashed and indices->axes version of #24715. CC @barche

@barche
Copy link
Contributor

barche commented Dec 15, 2017

OK, thanks! I was contemplating giving this a gentle bump today, but you beat me to it :)

Just to be sure: the addition of the CartesianToLinear type (and its name) is OK then? I personally feel a better name could be found, but I can't think of anything.

@timholy
Copy link
Member Author

timholy commented Dec 15, 2017

I'm OK with it, and nothing dramatically better occurs to me either.

@StefanKarpinski
Copy link
Member

The name seems clear enough to me. Would it be at all helpful to have a LinearToCartesian type? Partly, my brain wants it for symmetry and partly I wonder if writing linear indexing operations by explicitly dispatching via a LinearToCartesian object might help simlify/clarify some code?

@timholy
Copy link
Member Author

timholy commented Dec 15, 2017

(obj::CartesianRange)[i] is exactly that. If we want the symmetric name perhaps it should be (obj::LinearRange)[i, j, k, ...].

@StefanKarpinski
Copy link
Member

Or the other option would be to rename CartesianRange to LinearToCartesian...

@timholy
Copy link
Member Author

timholy commented Dec 15, 2017

But its main role is as an iterator, where LinearToCartesian seems like a weird choice.

@StefanKarpinski
Copy link
Member

Ok, then LinearRange seems like it would be a better name (for symmetry).

@barche
Copy link
Contributor

barche commented Dec 15, 2017

I kind of like LinearRange, but I think LinearArray captures what it is slightly better, at the cost of not being symmetric. But at least it is a noun, like most other types.

@timholy
Copy link
Member Author

timholy commented Dec 15, 2017

Change to CartesianIterator and LinearIterator?

@timholy
Copy link
Member Author

timholy commented Dec 15, 2017

Though the problem is that neither of those captures the notion that you're not allowed to skip entries (this is the main distinction with the more general product iterator). CartesianUnitRange would be even better in that regard...

@barche
Copy link
Contributor

barche commented Dec 15, 2017

Since they both are arrays, maybe CartesianArray and LinearArray? But I'm not sure renaming CartesianRange is necessary, I prefer not renaming existing things just for the sake of renaming, if symmetry is important I prefer CartesianRange and LinearRange most.

@timholy
Copy link
Member Author

timholy commented Dec 15, 2017

OK, let's go with LinearRange.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Dec 15, 2017

So the doc string for this starts with

Define a region R spanning a multidimensional rectangular range of integer indices.

So maybe CartesianRegion and LinearRegion?

@barche
Copy link
Contributor

barche commented Dec 15, 2017

Those sound fine too, except that I'm not convinced renaming CartesianRange is worth it, range being part of the definition too.

@timholy
Copy link
Member Author

timholy commented Dec 15, 2017

I have meetings over the next couple of hours, but if some consensus emerges I'll implement the changes this evening.

@StefanKarpinski
Copy link
Member

Even though Range is an abstract type, even with this change, CartesianRange is not a subtype of it. That suggests that the name is either a little misleading or perhaps there's some sense in which CartesianRange is a kind of Range and not just a kind of array?

@barche
Copy link
Contributor

barche commented Dec 15, 2017

Since Range{T} <: AbstractArray{T,1} (I admit I looked that up) and CartesianRange is N-dimensional, it definitely is not a Range. My concern with the rename is that it will force another deprecation on existing users, but I don't know it it's actually used that much outside Base?

@barche
Copy link
Contributor

barche commented Dec 15, 2017

Another idea: CartesianIndices and LinearIndices if we are to rename both.

@timholy
Copy link
Member Author

timholy commented Dec 15, 2017

Fundamentally it is a product of AbstractUnitRanges

@StefanKarpinski
Copy link
Member

My concern with the rename is that it will force another deprecation on existing users, but I don't know it it's actually used that much outside Base?

I think the usage is fairly limited and we can femtoclean this fairly easily, so let's just focus on getting the name right. To be honest, I think this is really useful functionality, but the concepts are a little difficult to wrap your head around – which is precisely the kind of situation where nailing the terminology and naming really helps. For me, understanding that these are objects that capture the transformation done by ind2sub and sub2ind as objects really helps me, but a good name would help too.

So let me see if I can explain it correctly... A CartesianRange is a compact representation of the n-tuple coordinates of a rectangular region of indices. By indexing into it linearly (or iterating it), one can find the cartesian coordinates corresponding to a linear index into that rectangular coordinate space. A CartesianToLinear object is a compact representation of the 1-dimensional coordinates of a dense region of linear indices. By indexing into it with an n-tuple of coordinates, one can find the linear index corresponding to a given multidimensional index. The first one seems ok, but the second one lacks something... It's missing the fact that it captures the dimension tuple and that it's really an inverse of a CartesianRange object.

Well, I'm not sure that was helpful but I can see why naming this is hard...

@barche
Copy link
Contributor

barche commented Dec 15, 2017

OK, so what about CartesianIndices and LinearIndices, since both objects are collections of indices and both can in fact be indexed both linearly and using cartesian indices?

@StefanKarpinski
Copy link
Member

CartesianIndices and LinearIndices are pretty decent. Let's see what @timholy thinks.

@timholy
Copy link
Member Author

timholy commented Dec 15, 2017

Those work for me. I can polish this up (or @barche can take it over again, whoever gets to it first).

@StefanKarpinski
Copy link
Member

Makes sense... CartesianIndices is a collection of CartesianIndex objects; although LinearIndices is not a collection of LinearIndex objects, but it is a collection of linear indices (i.e. integers), so maybe that's cool? It's key behavior is that it maps a cartesian index to those.

@timholy timholy force-pushed the barche-cartesianrange-abstractarray branch from 116ae09 to cd67176 Compare December 16, 2017 10:48
@timholy timholy force-pushed the barche-cartesianrange-abstractarray branch from cd67176 to e261a40 Compare December 16, 2017 12:17
@timholy
Copy link
Member Author

timholy commented Dec 16, 2017

circleci seems hung. I'll merge this shortly barring objections.

@barche
Copy link
Contributor

barche commented Dec 16, 2017

Unless I'm not seeing this properly on my phone, I think news is not updated with the latest rename?

@timholy
Copy link
Member Author

timholy commented Dec 16, 2017

Good catch, thanks!

@timholy timholy merged commit 3eb5544 into master Dec 16, 2017
@timholy timholy deleted the barche-cartesianrange-abstractarray branch December 16, 2017 16:05
@timholy
Copy link
Member Author

timholy commented Dec 16, 2017

@barche, I also just wanted to say that I'm unreasonably happy about this change 🙂. This change just feels right, thanks for doing it!

@barche
Copy link
Contributor

barche commented Dec 17, 2017

@timholy Thanks, though I must say most improvements happened thanks to the useful comments after the initial PR, so for me this illustrates the value of collaborative development.

@GiggleLiu
Copy link
Contributor

GiggleLiu commented Sep 26, 2018

To help ancient Julia users migrate

The following expression is equivalent to ind2sub((10, 10), 18)

CartesianIndices((10, 10))[18]

The following expression is equivalent to sub2ind((10, 10), 2, 6)

LinearIndices((10, 10))[2, 6]

=============== Modification Note ===============
Modified according to @barche 's suggestion.

@timholy
Copy link
Member Author

timholy commented Sep 26, 2018

If you use julia-0.7 you get deprecation warnings that tell you this. But there are also easier syntaxes to remember:

cartind = CartesianIndices(A)  # A is your array
inds = cartind[18]

linind = LinearIndices(A)
i = linind[2, 6]

See also https://discourse.julialang.org/t/psa-replacement-of-ind2sub-sub2ind-in-julia-0-7/14666

@barche
Copy link
Contributor

barche commented Sep 26, 2018

And you can also omit the 1: for one-based ranges:

CartesianIndices((10, 10))[18]

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

Successfully merging this pull request may close these issues.

4 participants