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

Reconsider negative indexing? #435

Open
droundy opened this issue Mar 30, 2018 · 5 comments
Open

Reconsider negative indexing? #435

droundy opened this issue Mar 30, 2018 · 5 comments

Comments

@droundy
Copy link

droundy commented Mar 30, 2018

I've not yet used ndarray, but looking at its description, it sounds like it follows Python's approach of using negative indices to could back from the end of an array? I would strongly suggest reconsidering that. It is the feature of numpy that in my experience most commonly results in hard to find bugs for now users.

@jturner314
Copy link
Member

jturner314 commented Mar 30, 2018

The current status is:

  • Negative indices are supported in the various .slice*() methods.
  • Negative indices are not supported anywhere else, although I've been planning to change that.

My thoughts on this are:

  • It is important to make indexing from the end of the axis a convenient operation, because it is quite common.

  • Rust has a significant advantage over Python in this case: Rust has unsigned integer primitives as part of the core language. If you use usize to index an array, you don't have to worry about negative indices, because usize can't be negative.

  • Allowing negative indices is a nice way to handle indexing from the end of an axis, but it's not the only convenient way to do so. For example, we could support something like .slice(s![3..5, ..END-7]) or .slice(s![3..5, ..back(7)]).

  • I don't have a strong preference for negative indices over something a little more verbose, but negative indices are simpler to implement, and Rust's usize type seems sufficient to avoid bugs related to negative indices.

@droundy Do you agree that usize makes this not much of a concern?

@bluss and others: What are your thoughts on this?

Edit: I realized that while we could have .slice(s![3..5, 1..END-7]) or .slice(s![3..5, 1..back(7)]), the implementation would be pretty hacky and would prevent us from catching some other types of bugs. A cleaner implementation would require .slice(s![3;5, 1;END-7]) or .slice(s![3..5, front(1)..back(7)]), neither of which I like very much.

@droundy
Copy link
Author

droundy commented Mar 30, 2018

I agree that allowing usize would greatly reduce the danger of allowing negative indices.

Personally, I'd still prefer a solution for counting backwards that is more like normal rust.

@jturner314
Copy link
Member

Just to clarify, you can currently use usize everywhere; that's already implemented.

@ExpHP
Copy link
Contributor

ExpHP commented Apr 28, 2018

My lousy 2 cents, one month later:

  • The biggest footgun of indexing with actual negative indices is the fact that -0 is still 0. (rather than referring to the point just past the end)
  • Negative strides with non-negative indices do not have this danger.

@noamraph
Copy link

Hi, I just want to say that I really like END-7. I'm using Python a lot, and I have had some bugs with negative indexes which weren't meant to be negative. I understand that requiring usize should prevent that, but I don't really like the requirement to use usize. In a program I wrote I had to write "as usize" many times, and it wasn't pleasant. OTOH, END-7 is very explicit in what it means, and also easy to type.

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

4 participants