-
Notifications
You must be signed in to change notification settings - Fork 321
Add .nth() method to Slice type #386
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
Conversation
536b704
to
8ea7d38
Compare
/// assert_eq!(Slice::new(1, None, 2).nth(2), Some(5)); | ||
/// | ||
/// // indices 3, 3, 3, ... | ||
/// assert_eq!(Slice::new(3, Some(5), 0).nth(1000), Some(3)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What meaning do we give to a step of 0? It used to be an error when using it for slicing, I'll have to check again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you actually call slice()
with a step of 0, then you'll get a panic. (See dimension::do_slice
.) You'll also get a panic when calling slice()
if the slice begin/end is out-of-range.
In contrast, nth()
doesn't panic in these cases. It's possible to change it to panic if step == 0
, without changing the function signature. To make it panic for out-of-range indices, it would need an argument for the length of the axis you're going to slice.
If you'd prefer it panic in those cases, I'd probably choose a different name (e.g. index()
) instead of nth()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for clarifying!
That sounds you have no particular behaviour in mind for 0, so we can choose one freely. I'm not too fond of mixing panics in explicitly checked or fallible methods either, so I'm not sure. I do lean towards that it should be a panic -- maybe a debug assertion. Maybe indeed a debug assertion already in Slice::new ? Or is that redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah with public fields a debug assertion in new doesn't do much good anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's certainly possible to make the fields non-public (or maybe just step
) and add an assertion to new()
and step_by()
. In some ways, I think that's a better design anyway; what do you think? It's still not possible to handle out-of-range indices this way, but it would catch step == 0
early.
If we make some of the fields in Slice
non-public, I would also change enum SliceOrIndex { Slice { start, end, step }, ... }
to enum SliceOrIndex { Slice(Slice), ... }
to hide the fields there too.
Edit: If it matters, Python's slice
type does allow step == 0
.
I just realized that the implementation of |
As I mentioned on IRC, I realized that it's not possible to correctly handle negative |
This PR adds an
nth()
method for theSlice
type.Instead of adding an
nth()
method, it would be possible to implementIntoIterator
forSlice
and specializenth()
, but I think it's useful to providenth()
onSlice
itself. It would be nice to implementIndex
forSlice
instead ofnth()
, but I don't think that's possible becauseIndex::index()
must return a reference.I was planning to include a
len()
method as part of this PR, but after thinking about it some more, I don't see a good use-case forlen()
.If you don't think the
nth()
method is sufficiently useful to include inndarray
, that's fine too. I just found it useful in my own code and thought I'd propose it.