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

Announcing IdentityRanges #1

Closed
timholy opened this issue Mar 15, 2017 · 6 comments
Closed

Announcing IdentityRanges #1

timholy opened this issue Mar 15, 2017 · 6 comments

Comments

@timholy
Copy link
Member

timholy commented Mar 15, 2017

To watchers of JuliaArrays: here's a new package that makes it easy to "preserve" the indices of views. Please see the README. Any comments about implementation or tests would be of interest.

On a related note, there's an API question I'm struggling with, encapsulated by these tests currently marked broken (and which would be fixed if I uncommented this code). The question is, how should we "spell" the concept "please preserve the data but strip away the indices, returning something with indices that start with 1"? The clearest way to spell this is

dest = Array{eltype(src)}(map(length, indices(src)))
copy!(dest, CartesianRange(indices(dest)), src, CartesianRange(indices(src)))

But I'm wondering whether we want convert(Array, a) to be shorthand for that. The alternative viewpoint (the one we have now) is that convert(Array, a) should succeed only if the indices of a start with 1. Feedback highly desired. Currently I lean towards the latter (more conservative) approach but I am noticing the lack of a convenient way of spelling this operation.

There's also one test marked broken which appears to be a Julia bug (affecting IndexLinear SubArrays with non-1 indices). I haven't yet had time to dig into it, but will fix.

@mbauman
Copy link
Member

mbauman commented Mar 15, 2017

Just curious — how do you view this in relation to Base.Slice? Is this meant to be substantially similar, but an official API with support for 0.5?

As far as your opposite of the indices function, I think I'd go for collect:

Return an Array of all items in a collection or iterator.

@timholy
Copy link
Member Author

timholy commented Mar 15, 2017

Hah, I never really grokked that you could use Slice for this purpose. Nice! (EDIT: I like Slice a lot better now that I get it. I wonder, though, if it would be better to rename it to IdentityRange and put it in ranges.jl?) I think we might not need this package, except (as you say) for julia 0.5 support.

As far as your inverse of the indices function, I think I'd go for collect ...

(complement, not inverse, right?) I thought of that. But the next sentence stopped me:

For associative collections, returns Pair{KeyType, ValType}.

I think of AbstractArrays as being a special type of associative collection, where the keys are implicitly the indices. By this logic, collect shouldn't be allowed to change the indices.

@mbauman
Copy link
Member

mbauman commented Mar 15, 2017

Yeah, I see your point on collect… but then by the letter of the law it should be an illegal operation to collect(OffsetArray(…)) since that cannot return an Array (unless it happens to be 1-indexed). Personally, I'd view collect to have more freedom to change indices than convert(Array, …).

I'd totally be on board with a better name for Slice and exporting it. I'm not sure IdentityRange is the right name, though. Upon seeing this package my first thought was that iterating over an IdentityRange would always return the same element. But I now see how the identity is within getindex. Maybe IndexRange or OffsetRange? Not sure those are better… just brainstorming.

@timholy
Copy link
Member Author

timholy commented Mar 16, 2017

Now that I've thought about it a bit more, I'm less worried about collect. For an associative collection, you can say that it creates new indices because the array of pairs is indexed differently from the original collection. So now I'm less worried. Interestingly, though, currently collect doesn't do this, because similar creates an array with the same indices, which for an OffsetArray ends up creating another OffsetArray. I wonder if we should think of this as a bug, and one that should be fixed for 0.6.

In addition to collect, there's at least a couple bugs with Base.Slice in 0.6:

julia> a = Base.Slice(-1:1)
Base.Slice(-1:1)

julia> a == -1:1  # this would be false with a = OffsetArray(-1:1, -1:1), because the indices have to match
true

julia> indices(reverse(a))  # I think this should return an array with the same indices
(Base.OneTo(3),)

But reassuringly, things like a + 1 fail with an error (which is far better than returning something that's wrong).

Maybe IndexRange or OffsetRange? Not sure those are better… just brainstorming.

To me, their fundamental property is r[i] == i, and it would be good to capture that in the name. If you don't like IdentityRange, maybe SelfRange? SelfIndexedRange? TrivialRange? ElidableRange? SelfComposedRange? IdempotentRange? (The last two because of r[r] == r). Maybe IdempotentRange is best?

@mbauman
Copy link
Member

mbauman commented Mar 16, 2017

I'm not terribly surprised that there are bugs with Slice — it's the very first offset array that has made its way into Base and I didn't create a thorough test-suite for it. I had been thinking it'd be hard to accidentally run into, but it is a direct output of to_indices (which is now exported). It'd be great to import this test suite into Base.

I'll let you drive the collect discussion/timeframe since I don't really use OffsetArrays. I never would have guessed I'd be the one to add the first offset array to Base!

Really IdentityRange isn't all that terrible. IdempotentRange may be slightly better, particularly if we make the constructor idempotent, too (which I think is fitting with its docstring).

@timholy
Copy link
Member Author

timholy commented Apr 17, 2017

I've decided it may be better/easier not to rely on Base.Slice, see the challenges discussed in JuliaLang/julia#21052. So I'm registering this, JuliaLang/METADATA.jl#8889.

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

2 participants