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

Describe problem with sentinel values for non-1 arrays #27

Merged
merged 1 commit into from
Feb 20, 2017

Conversation

timholy
Copy link
Member

@timholy timholy commented Feb 16, 2017

@nalimilan
Copy link
Member

Thanks. I like the discussion, but the Julep explicitly lists the issue of non-standard indices in the "out of scope" section. So either add the discussion there, or make a concrete suggestion which fits with the two general proposals and remove that mention.

That may not be as hard as it sounds: we can just decide that by default all functions return 1-based linear indices, but that they will all accept a type as first parameter to get a different kind of indices. The question is, should we keep returning 1-based linear indices by default, or n-based linear indices (with arbitrary n depending on the array type)?

@timholy
Copy link
Member Author

timholy commented Feb 16, 2017

we can just decide that by default all functions return 1-based linear indices

This runs into a problem for AbstractVectors, where it's hard to tell whether v[i] is supposed to be a 1-based linear index or the cartesian index. We have decided on the latter meaning; we might have to introduce a LinearIndex type if we want to be able to have it mean the former.

For any other dimensionality, it's fine.

But sure, I will change it more comprehensively so that it's not "out of scope." It's such a nice Julep for discussing this troublesome family of functions, and I think it's worth laying out all the issues on the table rather than having a separate Julep just for this one issue.

@timholy
Copy link
Member Author

timholy commented Feb 16, 2017

Wait, I don't think you do treat non-1 indexing; there's a section on linear vs cartesian indexes, but that's not the same thing. Can you point me to what you mean?

@nalimilan
Copy link
Member

This runs into a problem for AbstractVectors, where it's hard to tell whether v[i] is supposed to be a 1-based linear index or the cartesian index. We have decided on the latter meaning; we might have to introduce a LinearIndex type if we want to be able to have it mean the former.

I'm not sure I follow. Could you precise what you mean? I just suggested we do the same as for enumerate, i.e. return the 1-based position of the found entry by default, and return other kinds of indices when passing an index type as first argument.

Wait, I don't think you do treat non-1 indexing; there's a section on linear vs cartesian indexes, but that's not the same thing. Can you point me to what you mean?

Yes, I referred to that section. It doesn't deal with non-1 indices, but that's really the same issue from an API perspective AFAICT.

@timholy
Copy link
Member Author

timholy commented Feb 16, 2017

I'm not sure I follow. Could you precise what you mean? I just suggested we do the same as for enumerate, i.e. return the 1-based position of the found entry by default, and return other kinds of indices when passing an index type as first argument.

We've followed a firm rule that A[CartesianIndex((i,j))] does exactly the same thing as A[i,j]. For 1-d arrays, this means we can't easily distinguish cartesian indexing from linear indexing. For example:

julia> x = OffsetArray(1:9, -4:4)
OffsetArrays.OffsetArray{Int64,1,UnitRange{Int64}} with indices -4:4:
 1
 2
 3
 4
 5
 6
 7
 8
 9

julia> x[1]
6

Currently we don't have a LinearIndex type that would let 1 always be interpreted as the first index, no matter what. Relevant discussion about a potential way to unify these, and some of the negatives: JuliaLang/julia#20573 (comment).

Does this help? I think that also addresses the second point as to why I see these as quite separate issues.

@nalimilan
Copy link
Member

I see, thanks. So we need to decide what's the best default behavior; I think it would make sense to behave like enumerate for consistency and use 1-based indexing. Then we can discuss what IndexLinear/LinearIndex would mean when we add it.

@timholy
Copy link
Member Author

timholy commented Feb 16, 2017

I'll let you decide about merging this, I just didn't want this issue to be forgotten as folks think about the redesign.

@nalimilan
Copy link
Member

If you don't have the time to add a full proposal to fix this, could you just insert the paragraph in the "out of scope" section for now? As I said, the list where you added it currently only contains questions which are addressed by the two proposals, which isn't the case of this one.

@timholy
Copy link
Member Author

timholy commented Feb 20, 2017

I'm slow, but now I get it 😄.

@timholy timholy merged commit 800c84e into master Feb 20, 2017
@timholy timholy deleted the teh/find_indices branch February 20, 2017 12:12
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.

3 participants