implement creating SliceArg from arbitrary Dimension

[SparrowLii]

Fix #892
I added a field named in_dim to SliceInfo to indicate the entire length of the incides, which is the length of the SliceArg to be generated. Then the following implementation is done in slice.rs:

impl<T, D, Do> AsRef<SliceInfo<D::SliceArg, Do, D>> for SliceInfo<T, Do, D>
    where
        T: AsRef<[SliceOrIndex]>,
        Do: Dimension,
        D: Dimension,
{
    fn as_ref(&self) -> &SliceInfo<D::SliceArg, Do, D> {
        let index = self.indices.as_ref();
        if let Some(dim) = D::NDIM {
            debug_assert!(index.len() == dim);
        }
        let arg_ref = D::slice_arg_from(index);
        unsafe {
            // This is okay because the only non-zero-sized member of
            // `SliceInfo` is `indices`, so `&SliceInfo<[SliceOrIndex], D>`
            // should have the same bitwise representation as
            // `&[SliceOrIndex]`.
            &*(arg_ref as *const D::SliceArg
                as *const SliceInfo<D::SliceArg, Do, D>)
        }
    }
}

In this way, not only IxDyn, but SliceArg of other dimensions can also be generated from AsRef<[SliceOrIndex]>.
Then Impl<T, Do> AsRef<SliceInfo<[SliceOrIndex], Do, IxDyn>> for SliceInfo<T, Do, $dim> is to achieve the conversion of other dimensions to IxDyn

[bluss]

I don't really know the design of the existing code enough to review. Why is there a new type parameter on SliceInfo? 🙂

[jturner314]

I can review this when I have time. I am also curious about the new type parameter. [Edit: Oh, I think I understand why this is necessary. I'm not sure that providing more ways to manually construct SliceInfo is the best way to go, though. SliceInfo should be more of an implementation detail that's constructed primarily via the s![] macro. I think it would be better to improve the existing slicing API using s![] to support equivalents of NumPy's ... and np.newaxis.] [Edit2: For the particular case of #892, a better approach may be a method like the following: fn chunk_mut<I, E>(&mut self, start: I, shape: E) -> ArrayViewMut<'_, A, D> where I: NdIndex<D>, E: IntoDimension<Dim = D>, S: DataMut. Or, just tell users to create a view and make multiple calls to slice_axis_inplace.]

Fwiw, I wrote up some of the reasoning behind the current design here: Implementing Improved Slicing in ndarray 0.11.

[SparrowLii]

@jturner314 Thank you very much for reviewing my code! I have to say that I found a much better way. And it will not make any changes to the existing code @bluss —— Why don't we directly create the &SliceInfo we need from AsRef<[SliceOrIndex]>? Just like the implementation of slice.rs:371, &[SliceOrIndex] and &SliceInfo can be converted to each other without cost. If the length does not match, just let it simply report an error in slice_arg_from. All we need to do is specify the type of Do. This point may be improved (maybe refer to the realization of s![]). But for the particular case of #892, it is completely enough.

[bluss]

I'm unsure if this should be a method on Dimension. It could be, it's a bit wonky? This is a hidden method so it is then not intended to be public user-facing API.

[bluss]

This method should not be listed as the first method in the Dimension trait. It is not the most important method in the trait.

[jturner314]

We could do something like this:

use std::convert::{TryFrom, TryInto};

pub trait TryFromRef<T: ?Sized> {
    type Error;
    fn try_from_ref(orig: &T) -> Result<&Self, Self::Error>;
}

impl<T: ?Sized, U: ?Sized> TryFromRef<T> for U
where
    for<'a> &'a U: TryFrom<&'a T>,
{
    // Once Rust has const generics, we can implement `TryFromRef` separately
    // for `[SliceOrIndex]` and `[SliceOrIndex; N]`, and use
    // `TryFromSliceError` for the `[SliceOrIndex; N]` case.
    // Actually, we only really need to implement this trait for `N <= 6`, so we until
    // Rust has const generics, we could have separate impls for each size.
    type Error = ();

    fn try_from_ref(orig: &T) -> Result<&Self, Self::Error> {
        orig.try_into().map_err(|_| ())
    }
}

pub trait Dimension {
    ...
    type SliceArg: ?Sized + AsRef<[SliceOrIndex]> + TryFromRef<[SliceOrIndex]>;
    ...
}

although this is pretty awkward too.

[jturner314]

I took some time to think about this, and came to the following conclusions:

1. I really don't think the example in I would like a simpler way to create a SliceArg that matches an arbitrary Dimension #892 should be implemented using SliceInfo, even if we add the necessary conversions. Using SliceInfo for this case is awkward, and creating it from a Vec requires an unnecessary allocation (for the Vec). IMO, the best way to implement the example is like this:

   let mut v = x.view_mut();
   field
       .axes()
       .for_each(|ax| v.slice_axis_inplace(ax.axis(), Slice::from(2..2 + ax.len())));
   v.assign(field);

   I've created PR Suggest single-axis slicing methods for generic fns #912 to hopefully document this better.

2. Long-term, to support "ellipsis" and "new axis" functionality in slicing, the cleanest way to express the API will be something like this:

   pub fn slice<I>(&self, info: &I) -> ArrayView<'_, A, I::OutDim>
   where
       I: CanSlice<D>,
       S: Data,

   In other words, we'll no longer need the Dimension::SliceArg associated type, and the type of the thing containing the slice information doesn't need to be any particular type. We could allow SliceInfo to be created from a Vec and check the dimensionality in the constructor.

   Huh, I just discovered that PR Add support for inserting new axes while slicing #570 exists and is still open. I had forgotten about that.

[jturner314]

I added a couple of comments for how I think the conversion should be implemented, if we decide to add a conversion like this.

[jturner314]

This isn't correct, because the Do here is unconstrained and is not checked. Instead, we should add a method to SliceInfo:

impl<T: ?Sized, D> SliceInfo<T, D>
where
    T: AsRef<[SliceOrIndex]>,
    D: Dimension,
{
    pub fn for_dimensionality<E>(&self) -> Result<&SliceInfo<E::SliceArg, D>, ShapeError>
    where
        E: Dimension,
    {
        ...
    }
}

which makes sure that the D is correctly preserved.

[jturner314]

For what it's worth, we could do this instead to avoid the need for Dimension::slice_arg_from:

impl<T: ?Sized, D> SliceInfo<T, D>
where
    T: AsRef<[SliceOrIndex]>,
    D: Dimension,
{
    pub fn for_dimensionality<'a, E>(&'a self) -> &'a SliceInfo<E::SliceArg, D>
    where
        E: Dimension,
        &'a E::SliceArg: TryFrom<&'a [SliceOrIndex]>,
    {
        ...
    }
}

but that would make it awkward to use in generic functions because users would have to add the &'a E::SliceArg: TryFrom<&'a [SliceOrIndex]> bound to their functions.

[SparrowLii]

That's right, function should be put in SliceInfo. But I think it seems difficult to move slice_arg_from out of Dimension. try_into() needs to ensure that SliceArg implements From<&[SliceOrIndex]>. I think we should avoid adding additional restrictions on SliceArg.

[SparrowLii]

In other words, we'll no longer need the Dimension::SliceArg associated type, and the type of the thing containing the slice information doesn't need to be any particular type. We could allow SliceInfo to be created from a Vec and check the dimensionality in the constructor.

I think it might be difficult. If we implement CanSlice<D> for Vec<SliceOrIndex> , how do we determine its Outdim type?

[jturner314]

If we implement CanSlice<D> for Vec<SliceOrIndex> , how do we determine its Outdim type?

Oh, I meant that it would be possible to create a SliceInfo from a Vec, rather than implementing CanSlice for Vec<SliceOrIndex> directly. The user has to specify the input and output dimensions when manually constructing a SliceInfo without s![]. Like in your original proposal on this PR, SliceInfo has a new type parameter for the dimension of the array being sliced.

I've rebased #570 off the latest master. I propose merging #570 instead of this PR, because #570 happens to solve the problem addressed by this PR, via the SliceInfo::new method (in addition to the new functionality).

[SparrowLii]

I've rebased #570 off the latest master. I propose merging #570 instead of this PR, because #570 happens to solve the problem addressed by this PR, via the SliceInfo::new method (in addition to the new functionality).

That's OK for me. I would love to see this issue can be solved.

[bluss]

IIUC, #570 takes over this use case