Implement owning constructors taking dimensions and strides #60
Conversation
|
Seems nice. We need a simpler way than this to create f-order arrays too, later. I was thinking you maybe know how to handle that best. Can we use a |
I think a nice API for this could be to have
Yes it's probably better. I think my current tests may not be enough. Some strides may enable a same |
|
Right, accessing the same element under multiple indices is the way broadcasting is implemented, and it can only be allowed for read only array views, so not for these constructors. |
These need to be tested and integrated into the constructor with strides.
vbarrielle
force-pushed
the
stride_constructor
branch
from
cb75694 to
e1fef22
Compare
|
Cool stuff happening here. Say something here if you want me to look at it. I don't think we want to expose the helper functions in src/dimension.rs. |
Yeah I wasn't sure about exposing those either. Once thing that made me think they had to be exposed was that it allowed documenting clearly what kind of strides are allowed. Otherwise I think this is in a pretty good shape now, so you can review it. Depending on what you prefer I can un-expose the helper functions before a merge. |
vbarrielle
changed the title
| #[derive(Clone, Debug, PartialEq)] | ||
| pub enum StrideError { | ||
| /// stride leads to out of bounds indexing | ||
| OutOfBoundsStride, |
There was a problem hiding this comment.
Error looks good. I'd like to remove Stride from all the variant names.
There was a problem hiding this comment.
Can we combine Aliasing and Negative? I don't know if we need the details.
|
One thing for info: In the long run I hope to switch all dimension impls to be on fixed size arrays, so It would mean that in the future, arguments like dimension and stride will be provided as arrays. But I'm hopeful that we can bridge the transition with associated types and a trait, just like how One thing I'm thinking about is if we ever want to accept signed strides. Maybe not. Maybe that's mostly relevant "for raw parts" |
|
Let's skip exporting the helper functions. I want the least API liability :D |
|
Note to anyone following, we optimize for c-order arrays in arithmetic right now, but we will fix that down the line (and add fast cases for f-order too). |
|
The latest changes rewrites the StrideError variants and no longer exposes the |
Negative strides look useful if you want to reverse iteration on some axis (at least that's what I use them for in numpy). |
|
The |
| sorted.slice_mut().sort(); | ||
| let mut res = strides.clone(); | ||
| for (ind, val) in strides.slice().iter().enumerate() { | ||
| // cannot binary search in unsorted... |
There was a problem hiding this comment.
It should probably just be a linear search. Linear search is faster than binary search anway, for slices surprisingly long, up to 32 or more elements.
There was a problem hiding this comment.
I'd use .unwrap() since it's expected to never be None. No reason to insert a panic with formatting here, that's just awful in the generated code unfortunately.
Compare asm in release mode here: http://is.gd/UFzf2y
There was a problem hiding this comment.
Right, I originally thought .expect() would document the fact that it should never be None but it's better to not have dead formatting code.
There was a problem hiding this comment.
It's an inspiration to finally fix this issue at least 😄 rust-lang/rust/pull/31116
Linear search should be more performant than binary search for all reasonable use cases (never more than 30 dimensions). Removed the expect() because it will add dedicated panic and printing code that should never execute.
| .iter() | ||
| .enumerate() | ||
| .find(|&(_, &x)| x == val) | ||
| .unwrap().0; // cannot panic by construction |
There was a problem hiding this comment.
This is actually shorter with the iterator method position.
There was a problem hiding this comment.
Oh nice I didn't know about that method.
|
Ok everything looks good. I think we want to scrap the abbreviation "uchk", I want to remove that from ndarray (the other use is deprecated)! No worries, at this point I can do any adjustments after merge as well. I would probably rename StrideError::Aliased to something more generic too. |
|
Ok I'll leave the renames to you after the merge since you probably know better which names are more consistent with the rest of the API :) |
|
Thank you Rust 1.7 is adding .sort_by_key() to slices, which can be used in the stride sort scenario, in the future. |
Implement owning constructors taking dimensions and strides
Fixes #59.
This is a first draft of constructors taking strides into account. Tests are required, and I need to think it through a bit more to see if the checks are ok.