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

Add support for slicing with subviews #377

Merged
merged 11 commits into from
Nov 20, 2017

Conversation

jturner314
Copy link
Member

@jturner314 jturner314 commented Oct 30, 2017

This is a cleaned-up version of PR #369, and it fixes #215. The history is organized into self-contained commits, and each commit passes all tests.

The changes are:

  1. Disable CI for Rust 1.18.0. This allows us to use features in the latest stable Rust.
  2. Rename .isubview() to .subview_inplace().
  3. Rename .islice() to .slice_inplace().
  4. Rename the ndarray::si module to ndarray::slice since the Si type is removed.
  5. Add .slice_axis(), .slice_axis_mut(), and .slice_axis_inplace() methods to ArrayBase.
  6. Add do_slice() to ndarray::dimension, and remove Dimension::do_slices() because it's no longer needed.
  7. Move abs_index() from the dimension_trait module to the dimension module and change its len argument to type Ix.
  8. Add NDIM: Option<usize> associated constant to Dimension trait.
  9. Add zero_index_with_ndim() to Dimension trait.
  10. Add support for using individual indices (to indicate subviews) when slicing by:
    • Change Si into an enum SliceOrIndex.
    • Change the type of Dimension::SliceArg to [SliceOrIndex; n] or [SliceOrIndex].
    • Add a new type SliceInfo that stores an array/vec/slice of SliceOrIndex and the dimension type after slicing.
    • Change the argument type of the out-of-place .slice*() methods to &SliceInfo.
    • Add a .slice_move() method.
    • Update the s![] macro to return a &SliceInfo<[SliceOrIndex; n], Do>.
  11. Add more tests for slicing.

Of those, the user-visible changes are 2, 3, 5, 8, and 10. (Technically, 6 and 9 are too, but they're hidden from the docs.) Changes 2, 3, and 10 are breaking changes. (Technically, 6 is too, but do_slices() was hidden from the docs.)

In the future, it would be nice to make slicing more ergonomic in some cases where the user currently has to call .as_ref() on the &SliceInfo instance. (See test_slice_dyninput_array_fixed, test_slice_dyninput_array_dyn, test_slice_dyninput_vec_fixed, and test_slice_dyninput_vec_dyn in tests/array.rs.)

@bluss
Copy link
Member

bluss commented Oct 30, 2017

Very nice work! 💟

@jturner314
Copy link
Member Author

New changes

I just force-pushed two additional changes:

  1. SliceInfo now derives Debug.
  2. The s![] macro now only calls .step() when a step is specified following ;. This allows the user to use custom types with step sizes in the s![] macro as long as they implement From<Custom> for SliceOrIndex and SliceNextDim<_, _> for Custom. Before, the macro would always set the step size to 1 after converting to SliceOrIndex if no step size was specified.

Experiences using this PR, and a potential Slice type

I've been using a fork of ndarray with this PR merged for the past few days, and it's been working well. It turns out that I have found a use for the old Si type that was removed as part of this PR. I have code like this:

let len = 6;
let m = Array1::zeros(len);
let c = Array2::zeros((len, len));
let idx_foo = Slice(0, None, 2);
let idx_bar = Slice(1, None, 2);
azip!(mut m (m.slice_mut(s![idx_foo])), ... in { *m = ... });
azip!(mut m (m.slice_mut(s![idx_bar])), ... in { *m = ... });
azip!(mut c (c.slice_mut(s![idx_foo, idx_foo])), ... in { *c = ... });
azip!(mut c (c.slice_mut(s![idx_bar, idx_bar])), ... in { *c = ... });
azip!(mut c (c.slice_mut(s![idx_foo, idx_bar])), ... in { *c = ... });
azip!(mut c (c.slice_mut(s![idx_bar, idx_foo])), ... in { *c = ... });

where Slice is like the old Si type. idx_foo and idx_bar have meaningful names in my code. The reason why a Slice type is useful here is that arrays with different numbers of dimensions use the same slice indices along individual axes.

To make this work, I'm using the following (outside of ndarray):

#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub struct Slice(pub Ixs, pub Option<Ixs>, pub Ixs);

impl From<Slice> for SliceOrIndex {
    fn from(s: Slice) -> SliceOrIndex {
        SliceOrIndex::Slice(s.0, s.1, s.2)
    }
}

impl<D: Dimension> SliceNextDim<D, D::Larger> for Slice {
    fn next_dim(&self, _: PhantomData<D>) -> PhantomData<D::Larger> {
        PhantomData
    }
}

Do you think it's worth keeping a Slice type in ndarray? One additional place where a Slice type could be convenient is as the argument to the .slice_axis*() methods in this PR.

@bluss
Copy link
Member

bluss commented Nov 3, 2017

Indeed, it seems like a useful type. Note the merge conflict on the PR. I don't mind further rebases and edits on PRs.

@jturner314
Copy link
Member Author

I rebased the PR off of the latest master and added the Slice type. I didn't derive Copy for Slice because in a future PR it may be useful to implement Iterator for Slice. (See rust-lang/rust#27186.)

@bluss
Copy link
Member

bluss commented Nov 3, 2017

Please derive Copy for it.

If we go back to the Range situation, it could have been solved by making it implement IntoIterator, not Iterator.

@jturner314
Copy link
Member Author

Please derive Copy for it.

Okay, done.

If we go back to the Range situation, it could have been solved by making it implement IntoIterator, not Iterator.

Good point. I guess IntoIterator didn't exist back then.

@jturner314
Copy link
Member Author

jturner314 commented Nov 3, 2017

One more change I'm considering is adding field names (start, end, and step) to SliceOrIndex::Slice and Slice to make them more like std::ops::Range and to make the code easier to read. What do you think?

Edit: The third field name will have to be step_size if we keep the step() method, or we can use step and rename step() to step_by().

@bluss
Copy link
Member

bluss commented Nov 3, 2017

fields and methods can have the same name, fwiw, but it's good to dwell on the situation.

@jturner314
Copy link
Member Author

fields and methods can have the same name, fwiw

Woah, I had no idea. That would be confusing, though. Apparently you call a method with x.f(), and if f is field that's a closure, you can call it with (x.f)().

I think I'd like to use field names start, end, and step, and method name step_by().

@bluss
Copy link
Member

bluss commented Nov 4, 2017

that sounds fine

Also rename .step() to .step_by().
@bluss bluss merged commit a380737 into rust-ndarray:master Nov 20, 2017
@bluss
Copy link
Member

bluss commented Nov 20, 2017

Thanks a lot for this huge work, for working through it and putting it into such a well disposed form in this PR! 🌟 💯

@jturner314 jturner314 deleted the slice-subview4 branch November 20, 2017 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support subviews in slice s! syntax
2 participants