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 inserting new axes while slicing #570

Merged
merged 28 commits into from Mar 13, 2021

Conversation

jturner314
Copy link
Member

@jturner314 jturner314 commented Dec 10, 2018

This is equivalent to NumPy's np.newaxis feature.

A few notes:

  • Adding a CanSlice trait has the additional benefit that .as_ref() is no longer needed when slicing dynamic-dimensional arrays. (See Calling slice on a dynamic size array #501.)
  • I haven't made CanSlice public because I don't see any reason (except for docs) to expose it.
  • It would be possible to use something like _ instead of an expression that evaluates to NewAxis to signal a new axis in the s![] macro.

Fixes #895

@bluss
Copy link
Member

bluss commented Dec 10, 2018

Exciting! I'll look closer later

@jturner314
Copy link
Member Author

I got to thinking – instead of having separate SliceNextInDim and SliceNextOutDim traits, this approach may make more sense:

trait SliceArg {
    // Number of dimensions that this slicing argument consumes in the input array.
    type InDim: Dimension;
    // Number of dimensions that this slicing argument produces in the output array.
    type OutDim: Dimension;

    fn next_in_dim<D>(&self, _: PhantomData<D>) -> PhantomData<D::Out>
    where
        D: Dimension + DimAdd<Self::InDim>,
    {
        PhantomData
    }

    fn next_out_dim<D>(&self, _: PhantomData<D>) -> PhantomData<D::Out>
    where
        D: Dimension + DimAdd<Self::OutDim>,
    {
        PhantomData
    }
}

impl SliceArg for usize {
    type InDim = Ix1;
    type OutDim = Ix0;
}

impl SliceArg for NewAxis {
    type InDim = Ix0;
    type OutDim = Ix1;
}

impl SliceArg for Range<usize> {
    type InDim = Ix1;
    type OutDim = Ix1;
}

// ...

trait DimAdd<D: Dimension>: Dimension {
    type Out: Dimension;
}

// ...

impl DimAdd<Ix2> for Ix3 {
    type Out = Ix5;
}

// ...

impl DimAdd<IxDyn> for Ix3 {
    type Out = IxDyn;
}

// ...

impl<D: Dimension> DimAdd<D> for IxDyn {
    type Out = IxDyn;
}

// In s![] macro, to determine next input dimension use:
$crate::SliceArg::next_in_dim(&r, $in_dim)
// and to determine next output dimension use:
$crate::SliceArg::next_out_dim(&r, $out_dim)

This has a couple of advantages:

  • IMO the code is easier to read than SliceNextInDim/SliceNextOutDim implementations.
  • The DimAdd trait may be useful for other things too. I know I've wanted other dimension arithmetic traits in the past such as DimSub.

@jturner314 jturner314 mentioned this pull request Dec 13, 2018
src/slice.rs Outdated Show resolved Hide resolved
src/slice.rs Outdated Show resolved Hide resolved
src/slice.rs Outdated Show resolved Hide resolved
src/slice.rs Outdated Show resolved Hide resolved
src/dimension/mod.rs Outdated Show resolved Hide resolved
src/impl_methods.rs Outdated Show resolved Hide resolved
@bluss
Copy link
Member

bluss commented Dec 16, 2018

@jturner314 DimAdd looks like a fine idea

src/slice.rs Show resolved Hide resolved
src/slice.rs Outdated Show resolved Hide resolved
]
};

let c = range_mat64(n, q).into_shape(cdim).unwrap();

{
let mut av = a.slice_mut(s![..;s1, ..;s2]);
let c = c.slice(SliceInfo::<_, IxDyn>::new(cslice).unwrap().as_ref());
let c = c.slice(&SliceInfo::<_, IxDyn, IxDyn>::new(cslice).unwrap());
Copy link
Member

@bluss bluss Dec 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be fixed (in this PR or another), we can't design a library where SliceInfo::<_, IxDyn, IxDyn>::new(cslice).unwrap() is a normal thing to say.

I don't keep up with IxDyn, since I don't use it much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add

impl SliceInfo<Vec<AxisSliceInfo>, IxDyn, IxDyn> {
    pub fn from_vec(indices: Vec<AxisSliceInfo>) -> Result<Self, ShapeError> {
        SliceInfo::new(indices)
    }
}

or

impl<T> SliceInfo<T, IxDyn, IxDyn>
where
    T: AsRef<[AxisSliceInfo]>,
{
    pub fn new_dyn(indices: T) -> Result<Self, ShapeError> {
        SliceInfo::new(indices)
    }
}

I don't really expect anyone to create a SliceInfo instance directly, though, when they can just use the s![] macro or call .slice_axis()/.index_axis()/.insert_axis() on individual axes. Once we add support for Ellipsis (i.e. ...) in the s![] macro, there are even fewer use cases for creating SliceInfo directly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed a better solution. If we implement CanSlice<IxDyn> for [AxisSliceInfo], then there's no need for the user to create a SliceInfo instance here; they can just write &*cslice. (Optionally, we could also implement CanSlice<IxDyn> for Vec<AxisSliceInfo> to avoid the need for the *.)

We could also consider implementing CanSlice<IxDyn> for [Slice] and CanSlice<Dim<[usize; N]>> for [Slice; N], although that would require modifying the CanSlice trait somewhat (since it currently requires AsRef<[AxisSliceInfo]>) and the slicing method implementations.

I do think this test is better written by slicing individual axes with .slice_axis*() rather than combining the slicing information and calling .slice().

src/slice.rs Show resolved Hide resolved
src/slice.rs Outdated Show resolved Hide resolved
@bluss
Copy link
Member

bluss commented Dec 16, 2018

We have existing and new concepts and the dimensionality of each:

  • Slice (struct) 1D
  • AxisSliceInfo (enum) 1D
  • SliceInfo (struct) nD
  • CanSlice (trait) nD
  • NewAxis 1D

1D concepts are for a single axis and nD concepts pertain to the whole array.

NewAxis already has the perfect name, so it's fine. We again need to bring some order to this and give them the best names. I'll come back to this.

@jturner314 jturner314 mentioned this pull request Jul 25, 2019
@bluss
Copy link
Member

bluss commented Sep 12, 2019

I'm sorry that I haven't come back to this, and the PR doesn't need to wait for me.

@jturner314
Copy link
Member Author

jturner314 commented Feb 5, 2021

I've rebased this off the latest master. I think this is ready to merge, assuming the various type/trait names are fine. (See @bluss's comment above.)

The name CanSlice is a little awkward since we also have MultiSlice, but it's not a big deal because neither of those traits are available to users. (They're pub traits in private modules.)

@jturner314
Copy link
Member Author

Rebased again to resolve conflict in lib.rs in slicing example due to #913.

@bluss
Copy link
Member

bluss commented Feb 12, 2021

Docs is a good reason to make CanSlice visible in the docs. Users depend on clicking on it to understand what it is. (MultiSlice should be visible by the same logic).

Now, we don't have much of a module hierarchy in this crate. We're starting to miss it just to hide away lesser used things - suggestions welcome (with that said, I like the mostly-flat namespace).

This isn't much more code and simplifies the logic somewhat.
This makes it visible in the docs, but the private marker trick
prevents other crates from implementing it.
This makes it visible in the docs, but the private marker trick
prevents other crates from implementing it.
This reduces how often an explicit `DimAdd` bound is necessary.
@bluss bluss merged commit 09884fc into rust-ndarray:master Mar 13, 2021
@bluss
Copy link
Member

bluss commented Mar 13, 2021

Thanks a lot 🎉

@bluss
Copy link
Member

bluss commented Mar 13, 2021

With this done, it's time to release 0.15. Just need to know if there are any last-minute cleanups needed or necessary.

@jturner314 jturner314 deleted the slice-new-axis branch March 13, 2021 23:26
@jturner314
Copy link
Member Author

I'm glad this is merged! 🎉 I'm not aware of anything critical that needs to be done before the release. #919 and #936 are the two things that come to mind, but they could wait until 0.15.1 if necessary.

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.

trait MultiSlice is undocumented
2 participants