From 115b2af24e16eec43cfe13393e85f998a271582c Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 22:01:02 +0200 Subject: [PATCH 01/30] zip: In Zip, move dimension check to free function This reduces the generic parameters the function depends on, so that it is instantiated less often. --- src/zip/mod.rs | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/zip/mod.rs b/src/zip/mod.rs index 5f9b15c5a..34bfe523f 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -240,22 +240,25 @@ where } } -impl Zip +#[inline] +fn zip_dimension_check(dimension: &D, part: &P) where D: Dimension, + P: NdProducer, { - fn check

(&self, part: &P) - where - P: NdProducer, - { - ndassert!( - part.equal_dim(&self.dimension), - "Zip: Producer dimension mismatch, expected: {:?}, got: {:?}", - self.dimension, - part.raw_dim() - ); - } + ndassert!( + part.equal_dim(&dimension), + "Zip: Producer dimension mismatch, expected: {:?}, got: {:?}", + dimension, + part.raw_dim() + ); +} + +impl Zip +where + D: Dimension, +{ /// Return a the number of element tuples in the Zip pub fn size(&self) -> usize { self.dimension.size() @@ -652,7 +655,7 @@ macro_rules! map_impl { where P: IntoNdProducer, { let part = p.into_producer(); - self.check(&part); + zip_dimension_check(&self.dimension, &part); self.build_and(part) } From 13dab58ee564e067569cc73b1103dc21fe479511 Mon Sep 17 00:00:00 2001 From: bluss Date: Fri, 5 Mar 2021 20:56:58 +0100 Subject: [PATCH 02/30] append: Add try_append_row and try_append_column For Array2 specifically, and only when the memory layout lines up for efficient and simple append, allow appending rows and/or columns to an array. --- src/data_repr.rs | 38 +++++++++++ src/impl_owned_array.rs | 144 ++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 4 +- tests/append.rs | 84 +++++++++++++++++++++++ 4 files changed, 268 insertions(+), 2 deletions(-) create mode 100644 tests/append.rs diff --git a/src/data_repr.rs b/src/data_repr.rs index b34f0a4ca..8a52f64c4 100644 --- a/src/data_repr.rs +++ b/src/data_repr.rs @@ -6,6 +6,8 @@ use alloc::borrow::ToOwned; use alloc::vec::Vec; use crate::extension::nonnull; +use rawpointer::PointerExt; + /// Array's representation. /// /// *Don’t use this type directly—use the type alias @@ -55,6 +57,37 @@ impl OwnedRepr { self.ptr } + /// Return end pointer + pub(crate) fn as_end_nonnull(&self) -> NonNull { + unsafe { + self.ptr.add(self.len) + } + } + + /// Reserve `additional` elements; return the new pointer + /// + /// ## Safety + /// + /// Note that existing pointers into the data are invalidated + #[must_use = "must use new pointer to update existing pointers"] + pub(crate) fn reserve(&mut self, additional: usize) -> NonNull { + self.modify_as_vec(|mut v| { + v.reserve(additional); + v + }); + self.as_nonnull_mut() + } + + /// Set the valid length of the data + /// + /// ## Safety + /// + /// The first `new_len` elements of the data should be valid. + pub(crate) unsafe fn set_len(&mut self, new_len: usize) { + debug_assert!(new_len <= self.capacity); + self.len = new_len; + } + /// Cast self into equivalent repr of other element type /// /// ## Safety @@ -72,6 +105,11 @@ impl OwnedRepr { } } + fn modify_as_vec(&mut self, f: impl FnOnce(Vec) -> Vec) { + let v = self.take_as_vec(); + *self = Self::from(f(v)); + } + fn take_as_vec(&mut self) -> Vec { let capacity = self.capacity; let len = self.len; diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 41eff2b11..33d667778 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -1,6 +1,11 @@ use alloc::vec::Vec; + use crate::imp_prelude::*; +use crate::dimension; +use crate::error::{ErrorKind, ShapeError}; +use crate::OwnedRepr; +use crate::Zip; /// Methods specific to `Array0`. /// @@ -59,3 +64,142 @@ where self.data.into_vec() } } + +/// Methods specific to `Array2`. +/// +/// ***See also all methods for [`ArrayBase`]*** +/// +/// [`ArrayBase`]: struct.ArrayBase.html +impl Array { + /// Append a row to an array with row major memory layout. + /// + /// ***Errors*** with a layout error if the array is not in standard order or + /// if it has holes, even exterior holes (from slicing).
+ /// ***Errors*** with shape error if the length of the input row does not match + /// the length of the rows in the array.
+ /// + /// The memory layout matters, since it determines in which direction the array can easily + /// grow. Notice that an empty array is compatible both ways. The amortized average + /// complexity of the append is O(m) where *m* is the length of the row. + /// + /// ```rust + /// use ndarray::{Array, ArrayView, array}; + /// + /// // create an empty array and append + /// let mut a = Array::zeros((0, 4)); + /// a.try_append_row(ArrayView::from(&[ 1., 2., 3., 4.])).unwrap(); + /// a.try_append_row(ArrayView::from(&[-1., -2., -3., -4.])).unwrap(); + /// + /// assert_eq!( + /// a, + /// array![[ 1., 2., 3., 4.], + /// [-1., -2., -3., -4.]]); + /// ``` + pub fn try_append_row(&mut self, row: ArrayView) -> Result<(), ShapeError> + where + A: Clone, + { + let row_len = row.len(); + if row_len != self.len_of(Axis(1)) { + return Err(ShapeError::from_kind(ErrorKind::IncompatibleShape)); + } + let mut res_dim = self.raw_dim(); + res_dim[0] += 1; + let new_len = dimension::size_of_shape_checked(&res_dim)?; + + // array must be c-contiguous and be "full" (have no exterior holes) + if !self.is_standard_layout() || self.len() != self.data.len() { + return Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout)); + } + + unsafe { + // grow backing storage and update head ptr + debug_assert_eq!(self.data.as_ptr(), self.as_ptr()); + self.ptr = self.data.reserve(row_len); // because we are standard order + + // recompute strides - if the array was previously empty, it could have + // zeros in strides. + let strides = res_dim.default_strides(); + + // copy elements from view to the array now + // + // make a raw view with the new row + // safe because the data was "full" + let tail_ptr = self.data.as_end_nonnull(); + let tail_view = RawArrayViewMut::new(tail_ptr, Ix1(row_len), Ix1(1)); + + struct SetLenOnDrop<'a, A: 'a> { + len: usize, + data: &'a mut OwnedRepr, + } + + let mut length_guard = SetLenOnDrop { + len: self.data.len(), + data: &mut self.data, + }; + + impl Drop for SetLenOnDrop<'_, A> { + fn drop(&mut self) { + unsafe { + self.data.set_len(self.len); + } + } + } + + // assign the new elements + Zip::from(tail_view).and(row) + .for_each(|to, from| { + to.write(from.clone()); + length_guard.len += 1; + }); + + drop(length_guard); + + // update array dimension + self.strides = strides; + self.dim[0] += 1; + + } + // multiple assertions after pointer & dimension update + debug_assert_eq!(self.data.len(), self.len()); + debug_assert_eq!(self.len(), new_len); + debug_assert!(self.is_standard_layout()); + + Ok(()) + } + + /// Append a column to an array with column major memory layout. + /// + /// ***Errors*** with a layout error if the array is not in column major order or + /// if it has holes, even exterior holes (from slicing).
+ /// ***Errors*** with shape error if the length of the input column does not match + /// the length of the columns in the array.
+ /// + /// The memory layout matters, since it determines in which direction the array can easily + /// grow. Notice that an empty array is compatible both ways. The amortized average + /// complexity of the append is O(m) where *m* is the length of the column. + /// + /// ```rust + /// use ndarray::{Array, ArrayView, array}; + /// + /// // create an empty array and append + /// let mut a = Array::zeros((2, 0)); + /// a.try_append_column(ArrayView::from(&[1., 2.])).unwrap(); + /// a.try_append_column(ArrayView::from(&[-1., -2.])).unwrap(); + /// + /// assert_eq!( + /// a, + /// array![[1., -1.], + /// [2., -2.]]); + /// ``` + pub fn try_append_column(&mut self, column: ArrayView) -> Result<(), ShapeError> + where + A: Clone, + { + self.swap_axes(0, 1); + let ret = self.try_append_row(column); + self.swap_axes(0, 1); + ret + } +} + diff --git a/src/lib.rs b/src/lib.rs index c4a0812a3..802e6f0da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -235,8 +235,8 @@ pub type Ixs = isize; /// An *n*-dimensional array. /// -/// The array is a general container of elements. It cannot grow or shrink, but -/// can be sliced into subsets of its data. +/// The array is a general container of elements. It cannot grow or shrink (with some exceptions), +/// but can be sliced into subsets of its data. /// The array supports arithmetic operations by applying them elementwise. /// /// In *n*-dimensional we include for example 1-dimensional rows or columns, diff --git a/tests/append.rs b/tests/append.rs new file mode 100644 index 000000000..3b7a1179a --- /dev/null +++ b/tests/append.rs @@ -0,0 +1,84 @@ + +use ndarray::prelude::*; +use ndarray::{ShapeError, ErrorKind}; + +#[test] +fn append_row() { + let mut a = Array::zeros((0, 4)); + a.try_append_row(aview1(&[0., 1., 2., 3.])).unwrap(); + a.try_append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a.shape(), &[2, 4]); + + assert_eq!(a, + array![[0., 1., 2., 3.], + [4., 5., 6., 7.]]); + + assert_eq!(a.try_append_row(aview1(&[1.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); + assert_eq!(a.try_append_column(aview1(&[1.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); + assert_eq!(a.try_append_column(aview1(&[1., 2.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); +} + +#[test] +fn append_row_error() { + let mut a = Array::zeros((3, 4)); + + assert_eq!(a.try_append_row(aview1(&[1.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); + assert_eq!(a.try_append_column(aview1(&[1.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); + assert_eq!(a.try_append_column(aview1(&[1., 2., 3.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); +} + +#[test] +fn append_row_existing() { + let mut a = Array::zeros((1, 4)); + a.try_append_row(aview1(&[0., 1., 2., 3.])).unwrap(); + a.try_append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a.shape(), &[3, 4]); + + assert_eq!(a, + array![[0., 0., 0., 0.], + [0., 1., 2., 3.], + [4., 5., 6., 7.]]); + + assert_eq!(a.try_append_row(aview1(&[1.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); + assert_eq!(a.try_append_column(aview1(&[1.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); + assert_eq!(a.try_append_column(aview1(&[1., 2., 3.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); +} + +#[test] +fn append_row_col_len_1() { + // Test appending 1 row and then cols from shape 1 x 1 + let mut a = Array::zeros((1, 1)); + a.try_append_row(aview1(&[1.])).unwrap(); // shape 2 x 1 + a.try_append_column(aview1(&[2., 3.])).unwrap(); // shape 2 x 2 + assert_eq!(a.try_append_row(aview1(&[1.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); + assert_eq!(a.try_append_row(aview1(&[1., 2.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); + a.try_append_column(aview1(&[4., 5.])).unwrap(); // shape 2 x 3 + assert_eq!(a.shape(), &[2, 3]); + + assert_eq!(a, + array![[0., 2., 4.], + [1., 3., 5.]]); +} + +#[test] +fn append_column() { + let mut a = Array::zeros((4, 0)); + a.try_append_column(aview1(&[0., 1., 2., 3.])).unwrap(); + a.try_append_column(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a.shape(), &[4, 2]); + + assert_eq!(a.t(), + array![[0., 1., 2., 3.], + [4., 5., 6., 7.]]); +} From af47658e526dff4b596eb41ad5a4c31904d5516f Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 27 Mar 2021 21:49:13 +0100 Subject: [PATCH 03/30] append: .try_append_array() to append array to array Allow appending an array along an axis. This only succeeds if the axis is an appropriate growing axis, where this can be done without moving existing elements. --- src/impl_owned_array.rs | 140 ++++++++++++++++++++++++++++++++++++++++ tests/append.rs | 23 +++++++ 2 files changed, 163 insertions(+) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 33d667778..6a3fc0e49 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -203,3 +203,143 @@ impl Array { } } +impl Array + where D: Dimension +{ + /// Append a row to an array with row major memory layout. + /// + /// ***Errors*** with a layout error if the array is not in standard order or + /// if it has holes, even exterior holes (from slicing).
+ /// ***Errors*** with shape error if the length of the input row does not match + /// the length of the rows in the array.
+ /// + /// The memory layout matters, since it determines in which direction the array can easily + /// grow. Notice that an empty array is compatible both ways. The amortized average + /// complexity of the append is O(m) where *m* is the length of the row. + /// + /// ```rust + /// use ndarray::{Array, ArrayView, array}; + /// + /// // create an empty array and append + /// let mut a = Array::zeros((0, 4)); + /// a.try_append_row(ArrayView::from(&[1., 2., 3., 4.])).unwrap(); + /// a.try_append_row(ArrayView::from(&[0., -2., -3., -4.])).unwrap(); + /// + /// assert_eq!( + /// a, + /// array![[1., 2., 3., 4.], + /// [0., -2., -3., -4.]]); + /// ``` + pub fn try_append_array(&mut self, axis: Axis, array: ArrayView) + -> Result<(), ShapeError> + where + A: Clone, + D: RemoveAxis, + { + if self.ndim() == 0 { + return Err(ShapeError::from_kind(ErrorKind::IncompatibleShape)); + } + + let remaining_shape = self.raw_dim().remove_axis(axis); + let array_rem_shape = array.raw_dim().remove_axis(axis); + + if remaining_shape != array_rem_shape { + return Err(ShapeError::from_kind(ErrorKind::IncompatibleShape)); + } + + let len_to_append = array.len(); + if len_to_append == 0 { + return Ok(()); + } + + let array_shape = array.raw_dim(); + let mut res_dim = self.raw_dim(); + res_dim[axis.index()] += array_shape[axis.index()]; + let new_len = dimension::size_of_shape_checked(&res_dim)?; + + let self_is_empty = self.is_empty(); + + // array must be empty or have `axis` as the outermost (longest stride) + // axis + if !(self_is_empty || + self.axes().max_by_key(|ax| ax.stride).map(|ax| ax.axis) == Some(axis)) + { + return Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout)); + } + + // array must be be "full" (have no exterior holes) + if self.len() != self.data.len() { + return Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout)); + } + let strides = if self_is_empty { + // recompute strides - if the array was previously empty, it could have + // zeros in strides. + res_dim.default_strides() + } else { + self.strides.clone() + }; + + unsafe { + // grow backing storage and update head ptr + debug_assert_eq!(self.data.as_ptr(), self.as_ptr()); + self.ptr = self.data.reserve(len_to_append); // because we are standard order + + // copy elements from view to the array now + // + // make a raw view with the new row + // safe because the data was "full" + let tail_ptr = self.data.as_end_nonnull(); + let tail_view = RawArrayViewMut::new(tail_ptr, array_shape, strides.clone()); + + struct SetLenOnDrop<'a, A: 'a> { + len: usize, + data: &'a mut OwnedRepr, + } + + let mut length_guard = SetLenOnDrop { + len: self.data.len(), + data: &mut self.data, + }; + + impl Drop for SetLenOnDrop<'_, A> { + fn drop(&mut self) { + unsafe { + self.data.set_len(self.len); + } + } + } + + // we have a problem here XXX + // + // To be robust for panics and drop the right elements, we want + // to fill the tail in-order, so that we can drop the right elements on + // panic. Don't know how to achieve that. + // + // It might be easier to retrace our steps in a scope guard to drop the right + // elements.. (PartialArray style). + // + // assign the new elements + Zip::from(tail_view).and(array) + .for_each(|to, from| { + to.write(from.clone()); + length_guard.len += 1; + }); + + //length_guard.len += len_to_append; + dbg!(len_to_append); + drop(length_guard); + + // update array dimension + self.strides = strides; + self.dim = res_dim; + dbg!(&self.dim); + + } + // multiple assertions after pointer & dimension update + debug_assert_eq!(self.data.len(), self.len()); + debug_assert_eq!(self.len(), new_len); + debug_assert!(self.is_standard_layout()); + + Ok(()) + } +} diff --git a/tests/append.rs b/tests/append.rs index 3b7a1179a..4c0b093a2 100644 --- a/tests/append.rs +++ b/tests/append.rs @@ -82,3 +82,26 @@ fn append_column() { array![[0., 1., 2., 3.], [4., 5., 6., 7.]]); } + +#[test] +fn append_array1() { + let mut a = Array::zeros((0, 4)); + a.try_append_array(Axis(0), aview2(&[[0., 1., 2., 3.]])).unwrap(); + println!("{:?}", a); + a.try_append_array(Axis(0), aview2(&[[4., 5., 6., 7.]])).unwrap(); + println!("{:?}", a); + //a.try_append_column(aview1(&[4., 5., 6., 7.])).unwrap(); + //assert_eq!(a.shape(), &[4, 2]); + + assert_eq!(a, + array![[0., 1., 2., 3.], + [4., 5., 6., 7.]]); + + a.try_append_array(Axis(0), aview2(&[[5., 5., 4., 4.], [3., 3., 2., 2.]])).unwrap(); + println!("{:?}", a); + assert_eq!(a, + array![[0., 1., 2., 3.], + [4., 5., 6., 7.], + [5., 5., 4., 4.], + [3., 3., 2., 2.]]); +} From a7e3aab4e5e29123f653c66bba92f7c2372993aa Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 21:27:52 +0200 Subject: [PATCH 04/30] append: Update doc comment for try_append_array --- src/impl_owned_array.rs | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 6a3fc0e49..8d179dfbd 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -206,29 +206,50 @@ impl Array { impl Array where D: Dimension { - /// Append a row to an array with row major memory layout. + /// Append an array to the array + /// + /// The axis-to-append-to `axis` must be the array's "growing axis" for this operation + /// to succeed. The growing axis is the outermost or last-visited when elements are visited in + /// memory order: + /// + /// `axis` must be the growing axis of the current array, an axis with length 0 or 1. + /// + /// - This is the 0th axis for standard layout arrays + /// - This is the *n*-1 th axis for fortran layout arrays + /// - If the array is empty (the axis or any other has length 0) or if `axis` + /// has length 1, then the array can always be appended. /// /// ***Errors*** with a layout error if the array is not in standard order or /// if it has holes, even exterior holes (from slicing).
/// ***Errors*** with shape error if the length of the input row does not match /// the length of the rows in the array.
/// - /// The memory layout matters, since it determines in which direction the array can easily - /// grow. Notice that an empty array is compatible both ways. The amortized average - /// complexity of the append is O(m) where *m* is the length of the row. + /// The memory layout of the `self` array matters, since it determines in which direction the + /// array can easily grow. Notice that an empty array is compatible both ways. The amortized + /// average complexity of the append is O(m) where *m* is the number of elements in the + /// array-to-append (equivalent to how `Vec::extend` works). + /// + /// The memory layout of the argument `array` does not matter. /// /// ```rust - /// use ndarray::{Array, ArrayView, array}; + /// use ndarray::{Array, ArrayView, array, Axis}; /// /// // create an empty array and append /// let mut a = Array::zeros((0, 4)); - /// a.try_append_row(ArrayView::from(&[1., 2., 3., 4.])).unwrap(); - /// a.try_append_row(ArrayView::from(&[0., -2., -3., -4.])).unwrap(); + /// let ones = ArrayView::from(&[1.; 8]).into_shape((2, 4)).unwrap(); + /// let zeros = ArrayView::from(&[0.; 8]).into_shape((2, 4)).unwrap(); + /// a.try_append_array(Axis(0), ones).unwrap(); + /// a.try_append_array(Axis(0), zeros).unwrap(); + /// a.try_append_array(Axis(0), ones).unwrap(); /// /// assert_eq!( /// a, - /// array![[1., 2., 3., 4.], - /// [0., -2., -3., -4.]]); + /// array![[1., 1., 1., 1.], + /// [1., 1., 1., 1.], + /// [0., 0., 0., 0.], + /// [0., 0., 0., 0.], + /// [1., 1., 1., 1.], + /// [1., 1., 1., 1.]]); /// ``` pub fn try_append_array(&mut self, axis: Axis, array: ArrayView) -> Result<(), ShapeError> From f003aaa41a890a8aa6193a600240086ff273dff5 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 01:10:49 +0200 Subject: [PATCH 05/30] append: Solve axis iteration order problem by sorting axes --- src/impl_owned_array.rs | 75 ++++++++++++++++++++++++++++++++++------- src/zip/mod.rs | 7 ++++ tests/append.rs | 41 ++++++++++++++++++++++ 3 files changed, 110 insertions(+), 13 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 8d179dfbd..a7e127333 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -251,7 +251,7 @@ impl Array /// [1., 1., 1., 1.], /// [1., 1., 1., 1.]]); /// ``` - pub fn try_append_array(&mut self, axis: Axis, array: ArrayView) + pub fn try_append_array(&mut self, axis: Axis, mut array: ArrayView) -> Result<(), ShapeError> where A: Clone, @@ -310,7 +310,7 @@ impl Array // make a raw view with the new row // safe because the data was "full" let tail_ptr = self.data.as_end_nonnull(); - let tail_view = RawArrayViewMut::new(tail_ptr, array_shape, strides.clone()); + let mut tail_view = RawArrayViewMut::new(tail_ptr, array_shape, strides.clone()); struct SetLenOnDrop<'a, A: 'a> { len: usize, @@ -330,37 +330,86 @@ impl Array } } - // we have a problem here XXX - // // To be robust for panics and drop the right elements, we want // to fill the tail in-order, so that we can drop the right elements on - // panic. Don't know how to achieve that. + // panic. // - // It might be easier to retrace our steps in a scope guard to drop the right - // elements.. (PartialArray style). + // We have: Zip::from(tail_view).and(array) + // Transform tail_view into standard order by inverting and moving its axes. + // Keep the Zip traversal unchanged by applying the same axis transformations to + // `array`. This ensures the Zip traverses the underlying memory in order. // - // assign the new elements + // XXX It would be possible to skip this transformation if the element + // doesn't have drop. However, in the interest of code coverage, all elements + // use this code initially. + + if tail_view.ndim() > 1 { + for i in 0..tail_view.ndim() { + if tail_view.stride_of(Axis(i)) < 0 { + tail_view.invert_axis(Axis(i)); + array.invert_axis(Axis(i)); + } + } + sort_axes_to_standard_order(&mut tail_view, &mut array); + } Zip::from(tail_view).and(array) + .debug_assert_c_order() .for_each(|to, from| { to.write(from.clone()); length_guard.len += 1; }); - //length_guard.len += len_to_append; - dbg!(len_to_append); drop(length_guard); // update array dimension self.strides = strides; self.dim = res_dim; - dbg!(&self.dim); - } // multiple assertions after pointer & dimension update debug_assert_eq!(self.data.len(), self.len()); debug_assert_eq!(self.len(), new_len); - debug_assert!(self.is_standard_layout()); Ok(()) } } + +fn sort_axes_to_standard_order(a: &mut ArrayBase, b: &mut ArrayBase) +where + S: RawData, + S2: RawData, + D: Dimension, +{ + if a.ndim() <= 1 { + return; + } + sort_axes_impl(&mut a.dim, &mut a.strides, &mut b.dim, &mut b.strides); + debug_assert!(a.is_standard_layout()); +} + +fn sort_axes_impl(adim: &mut D, astrides: &mut D, bdim: &mut D, bstrides: &mut D) +where + D: Dimension, +{ + debug_assert!(adim.ndim() > 1); + debug_assert_eq!(adim.ndim(), bdim.ndim()); + // bubble sort axes + let mut changed = true; + while changed { + changed = false; + for i in 0..adim.ndim() - 1 { + let axis_i = i; + let next_axis = i + 1; + + // make sure higher stride axes sort before. + debug_assert!(astrides.slice()[axis_i] as isize >= 0); + if (astrides.slice()[axis_i] as isize) < astrides.slice()[next_axis] as isize { + changed = true; + adim.slice_mut().swap(axis_i, next_axis); + astrides.slice_mut().swap(axis_i, next_axis); + bdim.slice_mut().swap(axis_i, next_axis); + bstrides.slice_mut().swap(axis_i, next_axis); + } + } + } +} + diff --git a/src/zip/mod.rs b/src/zip/mod.rs index 34bfe523f..95f8381b8 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -673,6 +673,13 @@ macro_rules! map_impl { self.build_and(part) } + #[allow(unused)] + #[inline] + pub(crate) fn debug_assert_c_order(self) -> Self { + debug_assert!(self.layout.is(CORDER) || self.layout_tendency >= 0); + self + } + fn build_and

(self, part: P) -> Zip<($($p,)* P, ), D> where P: NdProducer, { diff --git a/tests/append.rs b/tests/append.rs index 4c0b093a2..f3323530d 100644 --- a/tests/append.rs +++ b/tests/append.rs @@ -105,3 +105,44 @@ fn append_array1() { [5., 5., 4., 4.], [3., 3., 2., 2.]]); } + +#[test] +fn append_array_3d() { + let mut a = Array::zeros((0, 2, 2)); + a.try_append_array(Axis(0), array![[[0, 1], [2, 3]]].view()).unwrap(); + println!("{:?}", a); + + let aa = array![[[51, 52], [53, 54]], [[55, 56], [57, 58]]]; + let av = aa.view(); + println!("Send {:?} to append", av); + a.try_append_array(Axis(0), av.clone()).unwrap(); + + a.swap_axes(0, 1); + let aa = array![[[71, 72], [73, 74]], [[75, 76], [77, 78]]]; + let mut av = aa.view(); + av.swap_axes(0, 1); + println!("Send {:?} to append", av); + a.try_append_array(Axis(1), av.clone()).unwrap(); + println!("{:?}", a); + let aa = array![[[81, 82], [83, 84]], [[85, 86], [87, 88]]]; + let mut av = aa.view(); + av.swap_axes(0, 1); + println!("Send {:?} to append", av); + a.try_append_array(Axis(1), av).unwrap(); + println!("{:?}", a); + assert_eq!(a, + array![[[0, 1], + [51, 52], + [55, 56], + [71, 72], + [75, 76], + [81, 82], + [85, 86]], + [[2, 3], + [53, 54], + [57, 58], + [73, 74], + [77, 78], + [83, 84], + [87, 88]]]); +} From 29896d8c2e829022d0d3621ed961c87e51fa43b7 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 02:08:55 +0200 Subject: [PATCH 06/30] append: Fix situations where we need to recompute stride When the axis has length 0, or 1, we need to carefully compute new strides. --- src/impl_owned_array.rs | 46 +++++++++++++++++++++++++++++--------- tests/append.rs | 49 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 10 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index a7e127333..c3cc0c6e1 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -261,6 +261,7 @@ impl Array return Err(ShapeError::from_kind(ErrorKind::IncompatibleShape)); } + let current_axis_len = self.len_of(axis); let remaining_shape = self.raw_dim().remove_axis(axis); let array_rem_shape = array.raw_dim().remove_axis(axis); @@ -280,22 +281,46 @@ impl Array let self_is_empty = self.is_empty(); - // array must be empty or have `axis` as the outermost (longest stride) - // axis - if !(self_is_empty || - self.axes().max_by_key(|ax| ax.stride).map(|ax| ax.axis) == Some(axis)) - { - return Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout)); + // array must be empty or have `axis` as the outermost (longest stride) axis + if !self_is_empty && current_axis_len > 1 { + // `axis` must be max stride axis or equal to its stride + let max_stride_axis = self.axes().max_by_key(|ax| ax.stride).unwrap(); + if max_stride_axis.axis != axis && max_stride_axis.stride > self.stride_of(axis) { + return Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout)); + } } // array must be be "full" (have no exterior holes) if self.len() != self.data.len() { return Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout)); } + let strides = if self_is_empty { - // recompute strides - if the array was previously empty, it could have - // zeros in strides. - res_dim.default_strides() + // recompute strides - if the array was previously empty, it could have zeros in + // strides. + // The new order is based on c/f-contig but must have `axis` as outermost axis. + if axis == Axis(self.ndim() - 1) { + // prefer f-contig when appending to the last axis + // Axis n - 1 is outermost axis + res_dim.fortran_strides() + } else { + // Default with modification + res_dim.slice_mut().swap(0, axis.index()); + let mut strides = res_dim.default_strides(); + res_dim.slice_mut().swap(0, axis.index()); + strides.slice_mut().swap(0, axis.index()); + strides + } + } else if current_axis_len == 1 { + // This is the outermost/longest stride axis; so we find the max across the other axes + let new_stride = self.axes().fold(1, |acc, ax| { + if ax.axis == axis { acc } else { + Ord::max(acc, ax.len as isize * ax.stride) + } + }); + let mut strides = self.strides.clone(); + strides[axis.index()] = new_stride as usize; + strides } else { self.strides.clone() }; @@ -383,7 +408,8 @@ where return; } sort_axes_impl(&mut a.dim, &mut a.strides, &mut b.dim, &mut b.strides); - debug_assert!(a.is_standard_layout()); + debug_assert!(a.is_standard_layout(), "not std layout dim: {:?}, strides: {:?}", + a.shape(), a.strides()); } fn sort_axes_impl(adim: &mut D, astrides: &mut D, bdim: &mut D, bstrides: &mut D) diff --git a/tests/append.rs b/tests/append.rs index f3323530d..a8a522a32 100644 --- a/tests/append.rs +++ b/tests/append.rs @@ -146,3 +146,52 @@ fn append_array_3d() { [83, 84], [87, 88]]]); } + +#[test] +fn test_append_2d() { + // create an empty array and append + let mut a = Array::zeros((0, 4)); + let ones = ArrayView::from(&[1.; 12]).into_shape((3, 4)).unwrap(); + let zeros = ArrayView::from(&[0.; 8]).into_shape((2, 4)).unwrap(); + a.try_append_array(Axis(0), ones).unwrap(); + a.try_append_array(Axis(0), zeros).unwrap(); + a.try_append_array(Axis(0), ones).unwrap(); + println!("{:?}", a); + assert_eq!(a.shape(), &[8, 4]); + for (i, row) in a.rows().into_iter().enumerate() { + let ones = i < 3 || i >= 5; + assert!(row.iter().all(|&x| x == ones as i32 as f64), "failed on lane {}", i); + } + + let mut a = Array::zeros((0, 4)); + a = a.reversed_axes(); + let ones = ones.reversed_axes(); + let zeros = zeros.reversed_axes(); + a.try_append_array(Axis(1), ones).unwrap(); + a.try_append_array(Axis(1), zeros).unwrap(); + a.try_append_array(Axis(1), ones).unwrap(); + println!("{:?}", a); + assert_eq!(a.shape(), &[4, 8]); + + for (i, row) in a.columns().into_iter().enumerate() { + let ones = i < 3 || i >= 5; + assert!(row.iter().all(|&x| x == ones as i32 as f64), "failed on lane {}", i); + } +} + +#[test] +fn test_append_middle_axis() { + // ensure we can append to Axis(1) by letting it become outermost + let mut a = Array::::zeros((3, 0, 2)); + a.try_append_array(Axis(1), Array::from_iter(0..12).into_shape((3, 2, 2)).unwrap().view()).unwrap(); + println!("{:?}", a); + a.try_append_array(Axis(1), Array::from_iter(12..24).into_shape((3, 2, 2)).unwrap().view()).unwrap(); + println!("{:?}", a); + + // ensure we can append to Axis(1) by letting it become outermost + let mut a = Array::::zeros((3, 1, 2)); + a.try_append_array(Axis(1), Array::from_iter(0..12).into_shape((3, 2, 2)).unwrap().view()).unwrap(); + println!("{:?}", a); + a.try_append_array(Axis(1), Array::from_iter(12..24).into_shape((3, 2, 2)).unwrap().view()).unwrap(); + println!("{:?}", a); +} From 54359a9c2ff39444bb03a9e84a7569e8693308b3 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 22:09:50 +0200 Subject: [PATCH 07/30] append: Use try_append_array for append row/column --- src/impl_owned_array.rs | 73 ++--------------------------------------- 1 file changed, 2 insertions(+), 71 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index c3cc0c6e1..cdab683c7 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -99,73 +99,7 @@ impl Array { where A: Clone, { - let row_len = row.len(); - if row_len != self.len_of(Axis(1)) { - return Err(ShapeError::from_kind(ErrorKind::IncompatibleShape)); - } - let mut res_dim = self.raw_dim(); - res_dim[0] += 1; - let new_len = dimension::size_of_shape_checked(&res_dim)?; - - // array must be c-contiguous and be "full" (have no exterior holes) - if !self.is_standard_layout() || self.len() != self.data.len() { - return Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout)); - } - - unsafe { - // grow backing storage and update head ptr - debug_assert_eq!(self.data.as_ptr(), self.as_ptr()); - self.ptr = self.data.reserve(row_len); // because we are standard order - - // recompute strides - if the array was previously empty, it could have - // zeros in strides. - let strides = res_dim.default_strides(); - - // copy elements from view to the array now - // - // make a raw view with the new row - // safe because the data was "full" - let tail_ptr = self.data.as_end_nonnull(); - let tail_view = RawArrayViewMut::new(tail_ptr, Ix1(row_len), Ix1(1)); - - struct SetLenOnDrop<'a, A: 'a> { - len: usize, - data: &'a mut OwnedRepr, - } - - let mut length_guard = SetLenOnDrop { - len: self.data.len(), - data: &mut self.data, - }; - - impl Drop for SetLenOnDrop<'_, A> { - fn drop(&mut self) { - unsafe { - self.data.set_len(self.len); - } - } - } - - // assign the new elements - Zip::from(tail_view).and(row) - .for_each(|to, from| { - to.write(from.clone()); - length_guard.len += 1; - }); - - drop(length_guard); - - // update array dimension - self.strides = strides; - self.dim[0] += 1; - - } - // multiple assertions after pointer & dimension update - debug_assert_eq!(self.data.len(), self.len()); - debug_assert_eq!(self.len(), new_len); - debug_assert!(self.is_standard_layout()); - - Ok(()) + self.try_append_array(Axis(0), row.insert_axis(Axis(0))) } /// Append a column to an array with column major memory layout. @@ -196,10 +130,7 @@ impl Array { where A: Clone, { - self.swap_axes(0, 1); - let ret = self.try_append_row(column); - self.swap_axes(0, 1); - ret + self.try_append_array(Axis(1), column.insert_axis(Axis(1))) } } From 6ca4275666ae47d8924fa6a5db998f084b9c87c3 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 22:10:57 +0200 Subject: [PATCH 08/30] stacking: Add a few more test cases for concatenate --- tests/stacking.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/stacking.rs b/tests/stacking.rs index 032525ffa..2c223fd66 100644 --- a/tests/stacking.rs +++ b/tests/stacking.rs @@ -1,4 +1,4 @@ -use ndarray::{arr2, arr3, aview1, concatenate, stack, Array2, Axis, ErrorKind, Ix1}; +use ndarray::{arr2, arr3, aview1, aview2, concatenate, stack, Array2, Axis, ErrorKind, Ix1}; #[test] fn concatenating() { @@ -15,6 +15,13 @@ fn concatenating() { let d = concatenate![Axis(0), a.row(0), &[9., 9.]]; assert_eq!(d, aview1(&[2., 2., 9., 9.])); + let d = concatenate![Axis(1), a.row(0).insert_axis(Axis(1)), aview1(&[9., 9.]).insert_axis(Axis(1))]; + assert_eq!(d, aview2(&[[2., 9.], + [2., 9.]])); + + let d = concatenate![Axis(0), a.row(0).insert_axis(Axis(1)), aview1(&[9., 9.]).insert_axis(Axis(1))]; + assert_eq!(d.t(), aview2(&[[2., 2., 9., 9.]])); + let res = ndarray::concatenate(Axis(1), &[a.view(), c.view()]); assert_eq!(res.unwrap_err().kind(), ErrorKind::IncompatibleShape); From e8629467bfdd9cd95e3abf55bf40f2ec65250634 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 22:11:35 +0200 Subject: [PATCH 09/30] stacking: Port concatenate to append, supporting Clone --- src/stacking.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/stacking.rs b/src/stacking.rs index 500ded6af..874c8cc4d 100644 --- a/src/stacking.rs +++ b/src/stacking.rs @@ -6,6 +6,9 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use alloc::vec::Vec; + +use crate::dimension; use crate::error::{from_kind, ErrorKind, ShapeError}; use crate::imp_prelude::*; @@ -68,7 +71,7 @@ where /// ``` pub fn concatenate(axis: Axis, arrays: &[ArrayView]) -> Result, ShapeError> where - A: Copy, + A: Clone, D: RemoveAxis, { if arrays.is_empty() { @@ -88,24 +91,21 @@ where let stacked_dim = arrays.iter().fold(0, |acc, a| acc + a.len_of(axis)); res_dim.set_axis(axis, stacked_dim); + let new_len = dimension::size_of_shape_checked(&res_dim)?; - // we can safely use uninitialized values here because we will - // overwrite every one of them. - let mut res = Array::uninit(res_dim); + // start with empty array with precomputed capacity + // try_append_array's handling of empty arrays makes sure `axis` is ok for appending + res_dim.set_axis(axis, 0); + let mut res = unsafe { + // Safety: dimension is size 0 and vec is empty + Array::from_shape_vec_unchecked(res_dim, Vec::with_capacity(new_len)) + }; - { - let mut assign_view = res.view_mut(); - for array in arrays { - let len = array.len_of(axis); - let (front, rest) = assign_view.split_at(axis, len); - array.assign_to(front); - assign_view = rest; - } - debug_assert_eq!(assign_view.len(), 0); - } - unsafe { - Ok(res.assume_init()) + for array in arrays { + res.try_append_array(axis, array.clone())?; } + debug_assert_eq!(res.len_of(axis), stacked_dim); + Ok(res) } #[deprecated(note="Use under the name stack instead.", since="0.15.0")] From 0159715fe4233a77b02b7e6103f363e685b3f44a Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 21:43:40 +0200 Subject: [PATCH 10/30] stacking: Port stack to append, supporting Clone --- src/stacking.rs | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/stacking.rs b/src/stacking.rs index 874c8cc4d..34bb61485 100644 --- a/src/stacking.rs +++ b/src/stacking.rs @@ -41,7 +41,7 @@ pub fn stack( arrays: &[ArrayView], ) -> Result, ShapeError> where - A: Copy, + A: Clone, D: Dimension, D::Larger: RemoveAxis, { @@ -138,7 +138,7 @@ pub fn stack_new_axis( arrays: &[ArrayView], ) -> Result, ShapeError> where - A: Copy, + A: Clone, D: Dimension, D::Larger: RemoveAxis, { @@ -158,24 +158,22 @@ where res_dim.set_axis(axis, arrays.len()); - // we can safely use uninitialized values here because we will - // overwrite every one of them. - let mut res = Array::uninit(res_dim); + let new_len = dimension::size_of_shape_checked(&res_dim)?; - res.axis_iter_mut(axis) - .zip(arrays.iter()) - .for_each(|(assign_view, array)| { - // assign_view is D::Larger::Smaller which is usually == D - // (but if D is Ix6, we have IxD != Ix6 here; differing types - // but same number of axes). - let assign_view = assign_view.into_dimensionality::() - .expect("same-dimensionality cast"); - array.assign_to(assign_view); - }); + // start with empty array with precomputed capacity + // try_append_array's handling of empty arrays makes sure `axis` is ok for appending + res_dim.set_axis(axis, 0); + let mut res = unsafe { + // Safety: dimension is size 0 and vec is empty + Array::from_shape_vec_unchecked(res_dim, Vec::with_capacity(new_len)) + }; - unsafe { - Ok(res.assume_init()) + for array in arrays { + res.try_append_array(axis, array.clone().insert_axis(axis))?; } + + debug_assert_eq!(res.len_of(axis), arrays.len()); + Ok(res) } /// Stack arrays along the new axis. From 9ad7a0fbe91b166586123de3af09ebbda500adbb Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 21:44:59 +0200 Subject: [PATCH 11/30] stacking: .select() now supports A: Clone Now that concatenate supports Clonable elements (previous: only Copy), we can loosen the generic restriction on .select() as well. --- src/impl_methods.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 03ca09d74..3e893fcd8 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -873,7 +873,7 @@ where /// ``` pub fn select(&self, axis: Axis, indices: &[Ix]) -> Array where - A: Copy, + A: Clone, S: Data, D: RemoveAxis, { From 647715782d8e7381babce36cb81e03a4ccbc3711 Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 23:24:26 +0200 Subject: [PATCH 12/30] zip: Fix iteration order debug assertion in Zip We're trying to assert the iteration order is like C-contig or C-preferred. The old assertion tripped when combining these two arrays: - shape 1, 7 strides 7, 1 (layout: CFcf) - shape 1, 7 strides 1, 2 (layout: f) The result was that the first was judged to have "no preference" and second "leaning f", total "leaning f". The debug assertion tripped a false positive because the traversal is one-dimensional, so elements are still visited in the required order. The layout judgment for the second array should ideally be "CFcf" too, but then it needs an understanding of being one-dimensional with contiguous stride of 2, which is not implemented. --- src/zip/mod.rs | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/zip/mod.rs b/src/zip/mod.rs index 95f8381b8..ffc6a710a 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -430,6 +430,26 @@ where } } +impl Zip<(P1, P2), D> +where + D: Dimension, + P1: NdProducer, + P1: NdProducer, +{ + /// Debug assert traversal order is like c (including 1D case) + // Method placement: only used for binary Zip at the moment. + #[inline] + pub(crate) fn debug_assert_c_order(self) -> Self { + debug_assert!(self.layout.is(CORDER) || self.layout_tendency >= 0 || + self.dimension.slice().iter().filter(|&&d| d > 1).count() <= 1, + "Assertion failed: traversal is not c-order or 1D for \ + layout {:?}, tendency {}, dimension {:?}", + self.layout, self.layout_tendency, self.dimension); + self + } +} + + /* trait Offset : Copy { unsafe fn offset(self, off: isize) -> Self; @@ -673,13 +693,6 @@ macro_rules! map_impl { self.build_and(part) } - #[allow(unused)] - #[inline] - pub(crate) fn debug_assert_c_order(self) -> Self { - debug_assert!(self.layout.is(CORDER) || self.layout_tendency >= 0); - self - } - fn build_and

(self, part: P) -> Zip<($($p,)* P, ), D> where P: NdProducer, { From 0599fd28cc1e0797a6ee94d8ae66c424b7299cdd Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 23:48:47 +0200 Subject: [PATCH 13/30] append: Handle empty arrays in append Fix a problem with appending empty arrays (found by ndarray-rand's quickcheck thank you). The try_append_array method was returning a tad too early in this case. --- src/impl_owned_array.rs | 12 +++++++++--- tests/append.rs | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index cdab683c7..f35bc8c67 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -201,15 +201,21 @@ impl Array } let len_to_append = array.len(); - if len_to_append == 0 { - return Ok(()); - } let array_shape = array.raw_dim(); let mut res_dim = self.raw_dim(); res_dim[axis.index()] += array_shape[axis.index()]; let new_len = dimension::size_of_shape_checked(&res_dim)?; + if len_to_append == 0 { + // There are no elements to append and shapes are compatible: + // either the dimension increment is zero, or there is an existing + // zero in another axis in self. + debug_assert_eq!(self.len(), new_len); + self.dim = res_dim; + return Ok(()); + } + let self_is_empty = self.is_empty(); // array must be empty or have `axis` as the outermost (longest stride) axis diff --git a/tests/append.rs b/tests/append.rs index a8a522a32..bd9617d83 100644 --- a/tests/append.rs +++ b/tests/append.rs @@ -195,3 +195,22 @@ fn test_append_middle_axis() { a.try_append_array(Axis(1), Array::from_iter(12..24).into_shape((3, 2, 2)).unwrap().view()).unwrap(); println!("{:?}", a); } + +#[test] +fn test_append_zero_size() { + { + let mut a = Array::::zeros((0, 0)); + a.try_append_array(Axis(0), aview2(&[[]])).unwrap(); + a.try_append_array(Axis(0), aview2(&[[]])).unwrap(); + assert_eq!(a.len(), 0); + assert_eq!(a.shape(), &[2, 0]); + } + + { + let mut a = Array::::zeros((0, 0)); + a.try_append_array(Axis(1), ArrayView::from(&[]).into_shape((0, 1)).unwrap()).unwrap(); + a.try_append_array(Axis(1), ArrayView::from(&[]).into_shape((0, 1)).unwrap()).unwrap(); + assert_eq!(a.len(), 0); + assert_eq!(a.shape(), &[0, 2]); + } +} From 0c578aab1fa099bed1bfddbc53e52c4c6907a10f Mon Sep 17 00:00:00 2001 From: bluss Date: Sat, 3 Apr 2021 23:50:56 +0200 Subject: [PATCH 14/30] append: Add append test that deals with incorrect layout situations The existing code can already be used to clone an array into an appendable situation. I.e. even if a.try_append_array(axis, b) would error, we can always do this: let c = empty array c.try_append_array(axis, &a); c.try_append_array(axis, &b); --- tests/append.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/append.rs b/tests/append.rs index bd9617d83..37cf50287 100644 --- a/tests/append.rs +++ b/tests/append.rs @@ -21,6 +21,32 @@ fn append_row() { Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); } +#[test] +fn append_row_wrong_layout() { + let mut a = Array::zeros((0, 4)); + a.try_append_row(aview1(&[0., 1., 2., 3.])).unwrap(); + a.try_append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a.shape(), &[2, 4]); + + assert_eq!(a.try_append_column(aview1(&[1., 2.])), + Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); + + assert_eq!(a, + array![[0., 1., 2., 3.], + [4., 5., 6., 7.]]); + + // Clone the array + + let mut dim = a.raw_dim(); + dim[1] = 0; + let mut b = Array::zeros(dim); + b.try_append_array(Axis(1), a.view()).unwrap(); + assert_eq!(b.try_append_column(aview1(&[1., 2.])), Ok(())); + assert_eq!(b, + array![[0., 1., 2., 3., 1.], + [4., 5., 6., 7., 2.]]); +} + #[test] fn append_row_error() { let mut a = Array::zeros((3, 4)); From aad9c7418dd20c09c0bfdffb659818133422edbb Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 4 Apr 2021 04:29:24 +0200 Subject: [PATCH 15/30] stacking: Add benchmarks for .select() (based on append) --- benches/append.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 benches/append.rs diff --git a/benches/append.rs b/benches/append.rs new file mode 100644 index 000000000..5502fa062 --- /dev/null +++ b/benches/append.rs @@ -0,0 +1,24 @@ +#![feature(test)] + +extern crate test; +use test::Bencher; + +use ndarray::prelude::*; + +#[bench] +fn select_axis0(bench: &mut Bencher) { + let a = Array::::zeros((256, 256)); + let selectable = vec![0, 1, 2, 0, 1, 3, 0, 4, 16, 32, 128, 147, 149, 220, 221, 255, 221, 0, 1]; + bench.iter(|| { + a.select(Axis(0), &selectable) + }); +} + +#[bench] +fn select_axis1(bench: &mut Bencher) { + let a = Array::::zeros((256, 256)); + let selectable = vec![0, 1, 2, 0, 1, 3, 0, 4, 16, 32, 128, 147, 149, 220, 221, 255, 221, 0, 1]; + bench.iter(|| { + a.select(Axis(1), &selectable) + }); +} From 3d9a9ba1dafc83fb821f41dc94b1809b9d458c1a Mon Sep 17 00:00:00 2001 From: bluss Date: Tue, 6 Apr 2021 21:31:30 +0200 Subject: [PATCH 16/30] move_into: New method .move_into() for moving all array elements .move_into() lets all elements move out of an Array, into an uninitialized array. We use a DropCounter to check duplication/drops of elements rigorously. The DropCounter code is taken from rayon collect tests, where I wrote it. --- src/impl_owned_array.rs | 187 ++++++++++++++++++++++++++++++- src/lib.rs | 1 + tests/assign.rs | 237 ++++++++++++++++++++++++++++++++++++++++ tests/higher_order_f.rs | 34 ------ 4 files changed, 423 insertions(+), 36 deletions(-) create mode 100644 tests/assign.rs diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index f35bc8c67..d367b8db2 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -1,9 +1,13 @@ use alloc::vec::Vec; +use std::mem::MaybeUninit; use crate::imp_prelude::*; + use crate::dimension; use crate::error::{ErrorKind, ShapeError}; +use crate::iterators::Baseiter; +use crate::low_level_util::AbortIfPanic; use crate::OwnedRepr; use crate::Zip; @@ -137,6 +141,145 @@ impl Array { impl Array where D: Dimension { + /// Move all elements from self into `new_array`, which must be of the same shape but + /// can have a different memory layout. The destination is overwritten completely. + /// + /// The destination should be a mut reference to an array or an `ArrayViewMut` with + /// `MaybeUninit` elements (which are overwritten without dropping any existing value). + /// + /// Minor implementation note: Owned arrays like `self` may be sliced in place and own elements + /// that are not part of their active view; these are dropped at the end of this function, + /// after all elements in the "active view" are moved into `new_array`. If there is a panic in + /// drop of any such element, other elements may be leaked. + /// + /// ***Panics*** if the shapes don't agree. + pub fn move_into<'a, AM>(self, new_array: AM) + where + AM: Into, D>>, + A: 'a, + { + // Remove generic parameter P and call the implementation + self.move_into_impl(new_array.into()) + } + + fn move_into_impl(mut self, new_array: ArrayViewMut, D>) { + unsafe { + // Safety: copy_to_nonoverlapping cannot panic + let guard = AbortIfPanic(&"move_into: moving out of owned value"); + // Move all reachable elements + Zip::from(self.raw_view_mut()) + .and(new_array) + .for_each(|src, dst| { + src.copy_to_nonoverlapping(dst.as_mut_ptr(), 1); + }); + guard.defuse(); + // Drop all unreachable elements + self.drop_unreachable_elements(); + } + } + + /// This drops all "unreachable" elements in the data storage of self. + /// + /// That means those elements that are not visible in the slicing of the array. + /// *Reachable elements are assumed to already have been moved from.* + /// + /// # Safety + /// + /// This is a panic critical section since `self` is already moved-from. + fn drop_unreachable_elements(mut self) -> OwnedRepr { + let self_len = self.len(); + + // "deconstruct" self; the owned repr releases ownership of all elements and we + // and carry on with raw view methods + let data_len = self.data.len(); + + let has_unreachable_elements = self_len != data_len; + if !has_unreachable_elements || std::mem::size_of::() == 0 { + unsafe { + self.data.set_len(0); + } + self.data + } else { + self.drop_unreachable_elements_slow() + } + } + + #[inline(never)] + #[cold] + fn drop_unreachable_elements_slow(mut self) -> OwnedRepr { + // "deconstruct" self; the owned repr releases ownership of all elements and we + // and carry on with raw view methods + let self_len = self.len(); + let data_len = self.data.len(); + let data_ptr = self.data.as_nonnull_mut().as_ptr(); + + let mut self_; + + unsafe { + // Safety: self.data releases ownership of the elements + self_ = self.raw_view_mut(); + self.data.set_len(0); + } + + + // uninvert axes where needed, so that stride > 0 + for i in 0..self_.ndim() { + if self_.stride_of(Axis(i)) < 0 { + self_.invert_axis(Axis(i)); + } + } + + // Sort axes to standard order, Axis(0) has biggest stride and Axis(n - 1) least stride + // Note that self_ has holes, so self_ is not C-contiguous + sort_axes_in_default_order(&mut self_); + + unsafe { + // with uninverted axes this is now the element with lowest address + let array_memory_head_ptr = self_.ptr.as_ptr(); + let data_end_ptr = data_ptr.add(data_len); + debug_assert!(data_ptr <= array_memory_head_ptr); + debug_assert!(array_memory_head_ptr <= data_end_ptr); + + // iter is a raw pointer iterator traversing self_ in its standard order + let mut iter = Baseiter::new(self_.ptr.as_ptr(), self_.dim, self_.strides); + let mut dropped_elements = 0; + + // The idea is simply this: the iterator will yield the elements of self_ in + // increasing address order. + // + // The pointers produced by the iterator are those that we *do not* touch. + // The pointers *not mentioned* by the iterator are those we have to drop. + // + // We have to drop elements in the range from `data_ptr` until (not including) + // `data_end_ptr`, except those that are produced by `iter`. + let mut last_ptr = data_ptr; + + while let Some(elem_ptr) = iter.next() { + // The interval from last_ptr up until (not including) elem_ptr + // should now be dropped. This interval may be empty, then we just skip this loop. + while last_ptr != elem_ptr { + debug_assert!(last_ptr < data_end_ptr); + std::ptr::drop_in_place(last_ptr); + last_ptr = last_ptr.add(1); + dropped_elements += 1; + } + // Next interval will continue one past the current element + last_ptr = elem_ptr.add(1); + } + + while last_ptr < data_end_ptr { + std::ptr::drop_in_place(last_ptr); + last_ptr = last_ptr.add(1); + dropped_elements += 1; + } + + assert_eq!(data_len, dropped_elements + self_len, + "Internal error: inconsistency in move_into"); + } + self.data + } + + /// Append an array to the array /// /// The axis-to-append-to `axis` must be the array's "growing axis" for this operation @@ -312,7 +455,7 @@ impl Array array.invert_axis(Axis(i)); } } - sort_axes_to_standard_order(&mut tail_view, &mut array); + sort_axes_to_standard_order_tandem(&mut tail_view, &mut array); } Zip::from(tail_view).and(array) .debug_assert_c_order() @@ -335,7 +478,21 @@ impl Array } } -fn sort_axes_to_standard_order(a: &mut ArrayBase, b: &mut ArrayBase) +/// Sort axes to standard order, i.e Axis(0) has biggest stride and Axis(n - 1) least stride +/// +/// The axes should have stride >= 0 before calling this method. +fn sort_axes_in_default_order(a: &mut ArrayBase) +where + S: RawData, + D: Dimension, +{ + if a.ndim() <= 1 { + return; + } + sort_axes1_impl(&mut a.dim, &mut a.strides); +} + +fn sort_axes_to_standard_order_tandem(a: &mut ArrayBase, b: &mut ArrayBase) where S: RawData, S2: RawData, @@ -349,6 +506,32 @@ where a.shape(), a.strides()); } +fn sort_axes1_impl(adim: &mut D, astrides: &mut D) +where + D: Dimension, +{ + debug_assert!(adim.ndim() > 1); + debug_assert_eq!(adim.ndim(), astrides.ndim()); + // bubble sort axes + let mut changed = true; + while changed { + changed = false; + for i in 0..adim.ndim() - 1 { + let axis_i = i; + let next_axis = i + 1; + + // make sure higher stride axes sort before. + debug_assert!(astrides.slice()[axis_i] as isize >= 0); + if (astrides.slice()[axis_i] as isize) < astrides.slice()[next_axis] as isize { + changed = true; + adim.slice_mut().swap(axis_i, next_axis); + astrides.slice_mut().swap(axis_i, next_axis); + } + } + } +} + + fn sort_axes_impl(adim: &mut D, astrides: &mut D, bdim: &mut D, bstrides: &mut D) where D: Dimension, diff --git a/src/lib.rs b/src/lib.rs index 802e6f0da..6a3f0a76b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,6 +12,7 @@ clippy::deref_addrof, clippy::unreadable_literal, clippy::manual_map, // is not an error + clippy::while_let_on_iterator, // is not an error )] #![cfg_attr(not(feature = "std"), no_std)] diff --git a/tests/assign.rs b/tests/assign.rs new file mode 100644 index 000000000..19156cce8 --- /dev/null +++ b/tests/assign.rs @@ -0,0 +1,237 @@ +use ndarray::prelude::*; + +use std::sync::atomic::{AtomicUsize, Ordering}; + +#[test] +fn assign() { + let mut a = arr2(&[[1., 2.], [3., 4.]]); + let b = arr2(&[[1., 3.], [2., 4.]]); + a.assign(&b); + assert_eq!(a, b); + + /* Test broadcasting */ + a.assign(&ArcArray::zeros(1)); + assert_eq!(a, ArcArray::zeros((2, 2))); + + /* Test other type */ + a.assign(&Array::from_elem((2, 2), 3.)); + assert_eq!(a, ArcArray::from_elem((2, 2), 3.)); + + /* Test mut view */ + let mut a = arr2(&[[1, 2], [3, 4]]); + { + let mut v = a.view_mut(); + v.slice_collapse(s![..1, ..]); + v.fill(0); + } + assert_eq!(a, arr2(&[[0, 0], [3, 4]])); +} + + +#[test] +fn assign_to() { + let mut a = arr2(&[[1., 2.], [3., 4.]]); + let b = arr2(&[[0., 3.], [2., 0.]]); + b.assign_to(&mut a); + assert_eq!(a, b); +} + +#[test] +fn move_into_copy() { + let a = arr2(&[[1., 2.], [3., 4.]]); + let acopy = a.clone(); + let mut b = Array::uninit(a.dim()); + a.move_into(b.view_mut()); + let b = unsafe { b.assume_init() }; + assert_eq!(acopy, b); + + let a = arr2(&[[1., 2.], [3., 4.]]).reversed_axes(); + let acopy = a.clone(); + let mut b = Array::uninit(a.dim()); + a.move_into(b.view_mut()); + let b = unsafe { b.assume_init() }; + assert_eq!(acopy, b); +} + +#[test] +fn move_into_owned() { + // Test various memory layouts and holes while moving String elements. + for &use_f_order in &[false, true] { + for &invert_axis in &[0b00, 0b01, 0b10, 0b11] { // bitmask for axis to invert + for &slice in &[false, true] { + let mut a = Array::from_shape_fn((5, 4).set_f(use_f_order), + |idx| format!("{:?}", idx)); + if slice { + a.slice_collapse(s![1..-1, ..;2]); + } + + if invert_axis & 0b01 != 0 { + a.invert_axis(Axis(0)); + } + if invert_axis & 0b10 != 0 { + a.invert_axis(Axis(1)); + } + + let acopy = a.clone(); + let mut b = Array::uninit(a.dim()); + a.move_into(b.view_mut()); + let b = unsafe { b.assume_init() }; + + assert_eq!(acopy, b); + } + } + } +} + +#[test] +fn move_into_slicing() { + // Count correct number of drops when using move_into and discontiguous arrays (with holes). + for &use_f_order in &[false, true] { + for &invert_axis in &[0b00, 0b01, 0b10, 0b11] { // bitmask for axis to invert + let counter = DropCounter::default(); + { + let (m, n) = (5, 4); + + let mut a = Array::from_shape_fn((m, n).set_f(use_f_order), |_idx| counter.element()); + a.slice_collapse(s![1..-1, ..;2]); + if invert_axis & 0b01 != 0 { + a.invert_axis(Axis(0)); + } + if invert_axis & 0b10 != 0 { + a.invert_axis(Axis(1)); + } + + let mut b = Array::uninit(a.dim()); + a.move_into(b.view_mut()); + let b = unsafe { b.assume_init() }; + + let total = m * n; + let dropped_1 = total - (m - 2) * (n - 2); + assert_eq!(counter.created(), total); + assert_eq!(counter.dropped(), dropped_1); + drop(b); + } + counter.assert_drop_count(); + } + } +} + +#[test] +fn move_into_diag() { + // Count correct number of drops when using move_into and discontiguous arrays (with holes). + for &use_f_order in &[false, true] { + let counter = DropCounter::default(); + { + let (m, n) = (5, 4); + + let a = Array::from_shape_fn((m, n).set_f(use_f_order), |_idx| counter.element()); + let a = a.into_diag(); + + let mut b = Array::uninit(a.dim()); + a.move_into(b.view_mut()); + let b = unsafe { b.assume_init() }; + + let total = m * n; + let dropped_1 = total - Ord::min(m, n); + assert_eq!(counter.created(), total); + assert_eq!(counter.dropped(), dropped_1); + drop(b); + } + counter.assert_drop_count(); + } +} + +#[test] +fn move_into_0dim() { + // Count correct number of drops when using move_into and discontiguous arrays (with holes). + for &use_f_order in &[false, true] { + let counter = DropCounter::default(); + { + let (m, n) = (5, 4); + + // slice into a 0-dim array + let a = Array::from_shape_fn((m, n).set_f(use_f_order), |_idx| counter.element()); + let a = a.slice_move(s![2, 2]); + + assert_eq!(a.ndim(), 0); + let mut b = Array::uninit(a.dim()); + a.move_into(b.view_mut()); + let b = unsafe { b.assume_init() }; + + let total = m * n; + let dropped_1 = total - 1; + assert_eq!(counter.created(), total); + assert_eq!(counter.dropped(), dropped_1); + drop(b); + } + counter.assert_drop_count(); + } +} + +#[test] +fn move_into_empty() { + // Count correct number of drops when using move_into and discontiguous arrays (with holes). + for &use_f_order in &[false, true] { + let counter = DropCounter::default(); + { + let (m, n) = (5, 4); + + // slice into an empty array; + let a = Array::from_shape_fn((m, n).set_f(use_f_order), |_idx| counter.element()); + let a = a.slice_move(s![..0, 1..1]); + assert!(a.is_empty()); + let mut b = Array::uninit(a.dim()); + a.move_into(b.view_mut()); + let b = unsafe { b.assume_init() }; + + let total = m * n; + let dropped_1 = total; + assert_eq!(counter.created(), total); + assert_eq!(counter.dropped(), dropped_1); + drop(b); + } + counter.assert_drop_count(); + } +} + + +/// This counter can create elements, and then count and verify +/// the number of which have actually been dropped again. +#[derive(Default)] +struct DropCounter { + created: AtomicUsize, + dropped: AtomicUsize, +} + +struct Element<'a>(&'a AtomicUsize); + +impl DropCounter { + fn created(&self) -> usize { + self.created.load(Ordering::Relaxed) + } + + fn dropped(&self) -> usize { + self.dropped.load(Ordering::Relaxed) + } + + fn element(&self) -> Element<'_> { + self.created.fetch_add(1, Ordering::Relaxed); + Element(&self.dropped) + } + + fn assert_drop_count(&self) { + assert_eq!( + self.created(), + self.dropped(), + "Expected {} dropped elements, but found {}", + self.created(), + self.dropped() + ); + } +} + +impl<'a> Drop for Element<'a> { + fn drop(&mut self) { + self.0.fetch_add(1, Ordering::Relaxed); + } +} diff --git a/tests/higher_order_f.rs b/tests/higher_order_f.rs index 1238cc4d8..c567eb3e0 100644 --- a/tests/higher_order_f.rs +++ b/tests/higher_order_f.rs @@ -6,37 +6,3 @@ fn test_fold_axis_oob() { let a = arr2(&[[1., 2.], [3., 4.]]); a.fold_axis(Axis(2), 0., |x, y| x + y); } - -#[test] -fn assign() { - let mut a = arr2(&[[1., 2.], [3., 4.]]); - let b = arr2(&[[1., 3.], [2., 4.]]); - a.assign(&b); - assert_eq!(a, b); - - /* Test broadcasting */ - a.assign(&ArcArray::zeros(1)); - assert_eq!(a, ArcArray::zeros((2, 2))); - - /* Test other type */ - a.assign(&Array::from_elem((2, 2), 3.)); - assert_eq!(a, ArcArray::from_elem((2, 2), 3.)); - - /* Test mut view */ - let mut a = arr2(&[[1, 2], [3, 4]]); - { - let mut v = a.view_mut(); - v.slice_collapse(s![..1, ..]); - v.fill(0); - } - assert_eq!(a, arr2(&[[0, 0], [3, 4]])); -} - - -#[test] -fn assign_to() { - let mut a = arr2(&[[1., 2.], [3., 4.]]); - let b = arr2(&[[0., 3.], [2., 0.]]); - b.assign_to(&mut a); - assert_eq!(a, b); -} From f153dc4662bb20f9e5c80b3e9c525dd9aa39bcc1 Mon Sep 17 00:00:00 2001 From: bluss Date: Tue, 6 Apr 2021 21:31:30 +0200 Subject: [PATCH 17/30] append: Adapt memory layout automatically in append if needed --- src/impl_owned_array.rs | 51 +++++++++++++++++++++++++++++++++++++++-- tests/append.rs | 25 ++++++++++++++------ 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index d367b8db2..f0512d530 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -279,6 +279,45 @@ impl Array self.data } + /// Create an empty array with an all-zeros shape + /// + /// ***Panics*** if D is zero-dimensional, because it can't be empty + pub(crate) fn empty() -> Array { + assert_ne!(D::NDIM, Some(0)); + let ndim = D::NDIM.unwrap_or(1); + Array::from_shape_simple_fn(D::zeros(ndim), || unreachable!()) + } + + /// Create new_array with the right layout for appending to `growing_axis` + #[cold] + fn change_to_contig_append_layout(&mut self, growing_axis: Axis) { + let ndim = self.ndim(); + let mut dim = self.raw_dim(); + + // The array will be created with 0 (C) or ndim-1 (F) as the biggest stride + // axis. Rearrange the shape so that `growing_axis` is the biggest stride axis + // afterwards. + let prefer_f_layout = growing_axis == Axis(ndim - 1); + if !prefer_f_layout { + dim.slice_mut().swap(0, growing_axis.index()); + } + let mut new_array = Self::uninit(dim.set_f(prefer_f_layout)); + if !prefer_f_layout { + new_array.swap_axes(0, growing_axis.index()); + } + + // self -> old_self. + // dummy array -> self. + // old_self elements are moved -> new_array. + let old_self = std::mem::replace(self, Self::empty()); + old_self.move_into(new_array.view_mut()); + + // new_array -> self. + unsafe { + *self = new_array.assume_init(); + } + } + /// Append an array to the array /// @@ -360,19 +399,27 @@ impl Array } let self_is_empty = self.is_empty(); + let mut incompatible_layout = false; // array must be empty or have `axis` as the outermost (longest stride) axis if !self_is_empty && current_axis_len > 1 { // `axis` must be max stride axis or equal to its stride let max_stride_axis = self.axes().max_by_key(|ax| ax.stride).unwrap(); if max_stride_axis.axis != axis && max_stride_axis.stride > self.stride_of(axis) { - return Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout)); + incompatible_layout = true; } } // array must be be "full" (have no exterior holes) if self.len() != self.data.len() { - return Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout)); + incompatible_layout = true; + } + + if incompatible_layout { + self.change_to_contig_append_layout(axis); + // safety-check parameters after remodeling + debug_assert_eq!(self_is_empty, self.is_empty()); + debug_assert_eq!(current_axis_len, self.len_of(axis)); } let strides = if self_is_empty { diff --git a/tests/append.rs b/tests/append.rs index 37cf50287..224fa6f3e 100644 --- a/tests/append.rs +++ b/tests/append.rs @@ -18,7 +18,10 @@ fn append_row() { assert_eq!(a.try_append_column(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); assert_eq!(a.try_append_column(aview1(&[1., 2.])), - Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); + Ok(())); + assert_eq!(a, + array![[0., 1., 2., 3., 1.], + [4., 5., 6., 7., 2.]]); } #[test] @@ -28,8 +31,7 @@ fn append_row_wrong_layout() { a.try_append_row(aview1(&[4., 5., 6., 7.])).unwrap(); assert_eq!(a.shape(), &[2, 4]); - assert_eq!(a.try_append_column(aview1(&[1., 2.])), - Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); + //assert_eq!(a.try_append_column(aview1(&[1., 2.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); assert_eq!(a, array![[0., 1., 2., 3.], @@ -56,7 +58,13 @@ fn append_row_error() { assert_eq!(a.try_append_column(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); assert_eq!(a.try_append_column(aview1(&[1., 2., 3.])), - Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); + Ok(())); + assert_eq!(a.t(), + array![[0., 0., 0.], + [0., 0., 0.], + [0., 0., 0.], + [0., 0., 0.], + [1., 2., 3.]]); } #[test] @@ -76,7 +84,11 @@ fn append_row_existing() { assert_eq!(a.try_append_column(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); assert_eq!(a.try_append_column(aview1(&[1., 2., 3.])), - Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); + Ok(())); + assert_eq!(a, + array![[0., 0., 0., 0., 1.], + [0., 1., 2., 3., 2.], + [4., 5., 6., 7., 3.]]); } #[test] @@ -87,8 +99,7 @@ fn append_row_col_len_1() { a.try_append_column(aview1(&[2., 3.])).unwrap(); // shape 2 x 2 assert_eq!(a.try_append_row(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); - assert_eq!(a.try_append_row(aview1(&[1., 2.])), - Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); + //assert_eq!(a.try_append_row(aview1(&[1., 2.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); a.try_append_column(aview1(&[4., 5.])).unwrap(); // shape 2 x 3 assert_eq!(a.shape(), &[2, 3]); From e0d07d31173bbcc6b62d6ca04c17fc353cef0d2c Mon Sep 17 00:00:00 2001 From: bluss Date: Wed, 7 Apr 2021 23:37:56 +0200 Subject: [PATCH 18/30] append: Handle negative stride arrays --- src/impl_owned_array.rs | 148 +++++++++++++++++++++++----------------- 1 file changed, 87 insertions(+), 61 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index f0512d530..926047501 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -2,6 +2,8 @@ use alloc::vec::Vec; use std::mem::MaybeUninit; +use rawpointer::PointerExt; + use crate::imp_prelude::*; use crate::dimension; @@ -332,15 +334,18 @@ impl Array /// - If the array is empty (the axis or any other has length 0) or if `axis` /// has length 1, then the array can always be appended. /// - /// ***Errors*** with a layout error if the array is not in standard order or - /// if it has holes, even exterior holes (from slicing).
- /// ***Errors*** with shape error if the length of the input row does not match - /// the length of the rows in the array.
+ /// ***Errors*** with shape error if the shape of self does not match the array-to-append; + /// all axes *except* the axis along which it being appended matter for this check. /// - /// The memory layout of the `self` array matters, since it determines in which direction the - /// array can easily grow. Notice that an empty array is compatible both ways. The amortized - /// average complexity of the append is O(m) where *m* is the number of elements in the - /// array-to-append (equivalent to how `Vec::extend` works). + /// The memory layout of the `self` array matters for ensuring that the append is efficient. + /// Appending automatically changes memory layout of the array so that it is appended to + /// along the "growing axis". + /// + /// Ensure appending is efficient by for example starting from an empty array and/or always + /// appending to an array along the same axis. + /// + /// The amortized average complexity of the append, when appending along its growing axis, is + /// O(*m*) where *m* is the length of the row. /// /// The memory layout of the argument `array` does not matter. /// @@ -404,8 +409,11 @@ impl Array // array must be empty or have `axis` as the outermost (longest stride) axis if !self_is_empty && current_axis_len > 1 { // `axis` must be max stride axis or equal to its stride - let max_stride_axis = self.axes().max_by_key(|ax| ax.stride).unwrap(); - if max_stride_axis.axis != axis && max_stride_axis.stride > self.stride_of(axis) { + let max_axis = self.axes().max_by_key(|ax| ax.stride.abs()).unwrap(); + if max_axis.axis != axis && max_axis.stride.abs() > self.stride_of(axis) { + incompatible_layout = true; + } + if self.stride_of(axis) < 0 { incompatible_layout = true; } } @@ -442,7 +450,8 @@ impl Array // This is the outermost/longest stride axis; so we find the max across the other axes let new_stride = self.axes().fold(1, |acc, ax| { if ax.axis == axis { acc } else { - Ord::max(acc, ax.len as isize * ax.stride) + let this_ax = ax.len as isize * ax.stride; + if this_ax.abs() > acc { this_ax } else { acc } } }); let mut strides = self.strides.clone(); @@ -454,26 +463,58 @@ impl Array unsafe { // grow backing storage and update head ptr - debug_assert_eq!(self.data.as_ptr(), self.as_ptr()); - self.ptr = self.data.reserve(len_to_append); // because we are standard order + let data_to_array_offset = if std::mem::size_of::() != 0 { + self.as_ptr().offset_from(self.data.as_ptr()) + } else { + 0 + }; + debug_assert!(data_to_array_offset >= 0); + self.ptr = self.data.reserve(len_to_append).offset(data_to_array_offset); - // copy elements from view to the array now + // clone elements from view to the array now + // + // To be robust for panics and drop the right elements, we want + // to fill the tail in memory order, so that we can drop the right elements on panic. // - // make a raw view with the new row - // safe because the data was "full" + // We have: Zip::from(tail_view).and(array) + // Transform tail_view into standard order by inverting and moving its axes. + // Keep the Zip traversal unchanged by applying the same axis transformations to + // `array`. This ensures the Zip traverses the underlying memory in order. + // + // XXX It would be possible to skip this transformation if the element + // doesn't have drop. However, in the interest of code coverage, all elements + // use this code initially. + + // Invert axes in tail_view by inverting strides + let mut tail_strides = strides.clone(); + if tail_strides.ndim() > 1 { + for i in 0..tail_strides.ndim() { + let s = tail_strides[i] as isize; + if s < 0 { + tail_strides.set_axis(Axis(i), -s as usize); + array.invert_axis(Axis(i)); + } + } + } + + // With > 0 strides, the current end of data is the correct base pointer for tail_view let tail_ptr = self.data.as_end_nonnull(); - let mut tail_view = RawArrayViewMut::new(tail_ptr, array_shape, strides.clone()); + let mut tail_view = RawArrayViewMut::new(tail_ptr, array_shape, tail_strides); + if tail_view.ndim() > 1 { + sort_axes_in_default_order_tandem(&mut tail_view, &mut array); + debug_assert!(tail_view.is_standard_layout(), + "not std layout dim: {:?}, strides: {:?}", + tail_view.shape(), tail_view.strides()); + } + + // Keep track of currently filled lenght of `self.data` and update it + // on scope exit (panic or loop finish). struct SetLenOnDrop<'a, A: 'a> { len: usize, data: &'a mut OwnedRepr, } - let mut length_guard = SetLenOnDrop { - len: self.data.len(), - data: &mut self.data, - }; - impl Drop for SetLenOnDrop<'_, A> { fn drop(&mut self) { unsafe { @@ -482,36 +523,18 @@ impl Array } } - // To be robust for panics and drop the right elements, we want - // to fill the tail in-order, so that we can drop the right elements on - // panic. - // - // We have: Zip::from(tail_view).and(array) - // Transform tail_view into standard order by inverting and moving its axes. - // Keep the Zip traversal unchanged by applying the same axis transformations to - // `array`. This ensures the Zip traverses the underlying memory in order. - // - // XXX It would be possible to skip this transformation if the element - // doesn't have drop. However, in the interest of code coverage, all elements - // use this code initially. + let mut data_length_guard = SetLenOnDrop { + len: self.data.len(), + data: &mut self.data, + }; - if tail_view.ndim() > 1 { - for i in 0..tail_view.ndim() { - if tail_view.stride_of(Axis(i)) < 0 { - tail_view.invert_axis(Axis(i)); - array.invert_axis(Axis(i)); - } - } - sort_axes_to_standard_order_tandem(&mut tail_view, &mut array); - } Zip::from(tail_view).and(array) .debug_assert_c_order() .for_each(|to, from| { to.write(from.clone()); - length_guard.len += 1; + data_length_guard.len += 1; }); - - drop(length_guard); + drop(data_length_guard); // update array dimension self.strides = strides; @@ -520,6 +543,7 @@ impl Array // multiple assertions after pointer & dimension update debug_assert_eq!(self.data.len(), self.len()); debug_assert_eq!(self.len(), new_len); + debug_assert!(self.pointer_is_inbounds()); Ok(()) } @@ -539,20 +563,6 @@ where sort_axes1_impl(&mut a.dim, &mut a.strides); } -fn sort_axes_to_standard_order_tandem(a: &mut ArrayBase, b: &mut ArrayBase) -where - S: RawData, - S2: RawData, - D: Dimension, -{ - if a.ndim() <= 1 { - return; - } - sort_axes_impl(&mut a.dim, &mut a.strides, &mut b.dim, &mut b.strides); - debug_assert!(a.is_standard_layout(), "not std layout dim: {:?}, strides: {:?}", - a.shape(), a.strides()); -} - fn sort_axes1_impl(adim: &mut D, astrides: &mut D) where D: Dimension, @@ -579,7 +589,23 @@ where } -fn sort_axes_impl(adim: &mut D, astrides: &mut D, bdim: &mut D, bstrides: &mut D) +/// Sort axes to standard order, i.e Axis(0) has biggest stride and Axis(n - 1) least stride +/// +/// Axes in a and b are sorted by the strides of `a`, and `a`'s axes should have stride >= 0 before +/// calling this method. +fn sort_axes_in_default_order_tandem(a: &mut ArrayBase, b: &mut ArrayBase) +where + S: RawData, + S2: RawData, + D: Dimension, +{ + if a.ndim() <= 1 { + return; + } + sort_axes2_impl(&mut a.dim, &mut a.strides, &mut b.dim, &mut b.strides); +} + +fn sort_axes2_impl(adim: &mut D, astrides: &mut D, bdim: &mut D, bstrides: &mut D) where D: Dimension, { From b09c770e1b76f6e4be774f91b047bbfd3bf49a51 Mon Sep 17 00:00:00 2001 From: bluss Date: Wed, 7 Apr 2021 18:50:42 +0200 Subject: [PATCH 19/30] append: Name methods .append(axis, array), .append_row/column() Now without the tricky memory layout failure mode, these methods can just have names without try_ prefix. It's expected that they work successfully as long as the user matches up array sizes correctly. --- src/impl_owned_array.rs | 72 ++++++++++++++++++------------- src/stacking.rs | 8 ++-- tests/append.rs | 94 ++++++++++++++++++++--------------------- 3 files changed, 93 insertions(+), 81 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 926047501..319e310a4 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -77,66 +77,78 @@ where /// /// [`ArrayBase`]: struct.ArrayBase.html impl Array { - /// Append a row to an array with row major memory layout. + /// Append a row to an array /// - /// ***Errors*** with a layout error if the array is not in standard order or - /// if it has holes, even exterior holes (from slicing).
- /// ***Errors*** with shape error if the length of the input row does not match - /// the length of the rows in the array.
+ /// ***Errors*** with a shape error if the length of the row does not match the length of the + /// rows in the array.
/// - /// The memory layout matters, since it determines in which direction the array can easily - /// grow. Notice that an empty array is compatible both ways. The amortized average - /// complexity of the append is O(m) where *m* is the length of the row. + /// The memory layout of the `self` array matters for ensuring that the append is efficient. + /// Appending automatically changes memory layout of the array so that it is appended to + /// along the "growing axis". + /// + /// Ensure appending is efficient by for example starting appending an empty array and + /// always appending along the same axis. For rows, ndarray's default layout is efficient for + /// appending. + /// + /// Notice that an empty array (where it has an axis of length zero) is the simplest starting + /// point. The amortized average complexity of the append is O(m) where *m* is the length of + /// the row. /// /// ```rust /// use ndarray::{Array, ArrayView, array}; /// /// // create an empty array and append /// let mut a = Array::zeros((0, 4)); - /// a.try_append_row(ArrayView::from(&[ 1., 2., 3., 4.])).unwrap(); - /// a.try_append_row(ArrayView::from(&[-1., -2., -3., -4.])).unwrap(); + /// a.append_row(ArrayView::from(&[ 1., 2., 3., 4.])).unwrap(); + /// a.append_row(ArrayView::from(&[-1., -2., -3., -4.])).unwrap(); /// /// assert_eq!( /// a, /// array![[ 1., 2., 3., 4.], /// [-1., -2., -3., -4.]]); /// ``` - pub fn try_append_row(&mut self, row: ArrayView) -> Result<(), ShapeError> + pub fn append_row(&mut self, row: ArrayView) -> Result<(), ShapeError> where A: Clone, { - self.try_append_array(Axis(0), row.insert_axis(Axis(0))) + self.append(Axis(0), row.insert_axis(Axis(0))) } - /// Append a column to an array with column major memory layout. + /// Append a column to an array + /// + /// ***Errors*** with a shape error if the length of the column does not match the length of + /// the columns in the array.
+ /// + /// The memory layout of the `self` array matters for ensuring that the append is efficient. + /// Appending automatically changes memory layout of the array so that it is appended to + /// along the "growing axis". /// - /// ***Errors*** with a layout error if the array is not in column major order or - /// if it has holes, even exterior holes (from slicing).
- /// ***Errors*** with shape error if the length of the input column does not match - /// the length of the columns in the array.
+ /// Ensure appending is efficient by for example starting appending an empty array and + /// always appending along the same axis. For columns, column major ("F") memory layout is + /// efficient for appending. /// - /// The memory layout matters, since it determines in which direction the array can easily - /// grow. Notice that an empty array is compatible both ways. The amortized average - /// complexity of the append is O(m) where *m* is the length of the column. + /// Notice that an empty array (where it has an axis of length zero) is the simplest starting + /// point. The amortized average complexity of the append is O(m) where *m* is the length of + /// the row. /// /// ```rust /// use ndarray::{Array, ArrayView, array}; /// /// // create an empty array and append /// let mut a = Array::zeros((2, 0)); - /// a.try_append_column(ArrayView::from(&[1., 2.])).unwrap(); - /// a.try_append_column(ArrayView::from(&[-1., -2.])).unwrap(); + /// a.append_column(ArrayView::from(&[1., 2.])).unwrap(); + /// a.append_column(ArrayView::from(&[-1., -2.])).unwrap(); /// /// assert_eq!( /// a, /// array![[1., -1.], /// [2., -2.]]); /// ``` - pub fn try_append_column(&mut self, column: ArrayView) -> Result<(), ShapeError> + pub fn append_column(&mut self, column: ArrayView) -> Result<(), ShapeError> where A: Clone, { - self.try_append_array(Axis(1), column.insert_axis(Axis(1))) + self.append(Axis(1), column.insert_axis(Axis(1))) } } @@ -334,7 +346,7 @@ impl Array /// - If the array is empty (the axis or any other has length 0) or if `axis` /// has length 1, then the array can always be appended. /// - /// ***Errors*** with shape error if the shape of self does not match the array-to-append; + /// ***Errors*** with a shape error if the shape of self does not match the array-to-append; /// all axes *except* the axis along which it being appended matter for this check. /// /// The memory layout of the `self` array matters for ensuring that the append is efficient. @@ -347,7 +359,7 @@ impl Array /// The amortized average complexity of the append, when appending along its growing axis, is /// O(*m*) where *m* is the length of the row. /// - /// The memory layout of the argument `array` does not matter. + /// The memory layout of the argument `array` does not matter to the same extent. /// /// ```rust /// use ndarray::{Array, ArrayView, array, Axis}; @@ -356,9 +368,9 @@ impl Array /// let mut a = Array::zeros((0, 4)); /// let ones = ArrayView::from(&[1.; 8]).into_shape((2, 4)).unwrap(); /// let zeros = ArrayView::from(&[0.; 8]).into_shape((2, 4)).unwrap(); - /// a.try_append_array(Axis(0), ones).unwrap(); - /// a.try_append_array(Axis(0), zeros).unwrap(); - /// a.try_append_array(Axis(0), ones).unwrap(); + /// a.append(Axis(0), ones).unwrap(); + /// a.append(Axis(0), zeros).unwrap(); + /// a.append(Axis(0), ones).unwrap(); /// /// assert_eq!( /// a, @@ -369,7 +381,7 @@ impl Array /// [1., 1., 1., 1.], /// [1., 1., 1., 1.]]); /// ``` - pub fn try_append_array(&mut self, axis: Axis, mut array: ArrayView) + pub fn append(&mut self, axis: Axis, mut array: ArrayView) -> Result<(), ShapeError> where A: Clone, diff --git a/src/stacking.rs b/src/stacking.rs index 34bb61485..fb05c1963 100644 --- a/src/stacking.rs +++ b/src/stacking.rs @@ -94,7 +94,7 @@ where let new_len = dimension::size_of_shape_checked(&res_dim)?; // start with empty array with precomputed capacity - // try_append_array's handling of empty arrays makes sure `axis` is ok for appending + // append's handling of empty arrays makes sure `axis` is ok for appending res_dim.set_axis(axis, 0); let mut res = unsafe { // Safety: dimension is size 0 and vec is empty @@ -102,7 +102,7 @@ where }; for array in arrays { - res.try_append_array(axis, array.clone())?; + res.append(axis, array.clone())?; } debug_assert_eq!(res.len_of(axis), stacked_dim); Ok(res) @@ -161,7 +161,7 @@ where let new_len = dimension::size_of_shape_checked(&res_dim)?; // start with empty array with precomputed capacity - // try_append_array's handling of empty arrays makes sure `axis` is ok for appending + // append's handling of empty arrays makes sure `axis` is ok for appending res_dim.set_axis(axis, 0); let mut res = unsafe { // Safety: dimension is size 0 and vec is empty @@ -169,7 +169,7 @@ where }; for array in arrays { - res.try_append_array(axis, array.clone().insert_axis(axis))?; + res.append(axis, array.clone().insert_axis(axis))?; } debug_assert_eq!(res.len_of(axis), arrays.len()); diff --git a/tests/append.rs b/tests/append.rs index 224fa6f3e..0e24a744d 100644 --- a/tests/append.rs +++ b/tests/append.rs @@ -5,19 +5,19 @@ use ndarray::{ShapeError, ErrorKind}; #[test] fn append_row() { let mut a = Array::zeros((0, 4)); - a.try_append_row(aview1(&[0., 1., 2., 3.])).unwrap(); - a.try_append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + a.append_row(aview1(&[0., 1., 2., 3.])).unwrap(); + a.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); assert_eq!(a.shape(), &[2, 4]); assert_eq!(a, array![[0., 1., 2., 3.], [4., 5., 6., 7.]]); - assert_eq!(a.try_append_row(aview1(&[1.])), + assert_eq!(a.append_row(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); - assert_eq!(a.try_append_column(aview1(&[1.])), + assert_eq!(a.append_column(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); - assert_eq!(a.try_append_column(aview1(&[1., 2.])), + assert_eq!(a.append_column(aview1(&[1., 2.])), Ok(())); assert_eq!(a, array![[0., 1., 2., 3., 1.], @@ -27,11 +27,11 @@ fn append_row() { #[test] fn append_row_wrong_layout() { let mut a = Array::zeros((0, 4)); - a.try_append_row(aview1(&[0., 1., 2., 3.])).unwrap(); - a.try_append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + a.append_row(aview1(&[0., 1., 2., 3.])).unwrap(); + a.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); assert_eq!(a.shape(), &[2, 4]); - //assert_eq!(a.try_append_column(aview1(&[1., 2.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); + //assert_eq!(a.append_column(aview1(&[1., 2.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); assert_eq!(a, array![[0., 1., 2., 3.], @@ -42,8 +42,8 @@ fn append_row_wrong_layout() { let mut dim = a.raw_dim(); dim[1] = 0; let mut b = Array::zeros(dim); - b.try_append_array(Axis(1), a.view()).unwrap(); - assert_eq!(b.try_append_column(aview1(&[1., 2.])), Ok(())); + b.append(Axis(1), a.view()).unwrap(); + assert_eq!(b.append_column(aview1(&[1., 2.])), Ok(())); assert_eq!(b, array![[0., 1., 2., 3., 1.], [4., 5., 6., 7., 2.]]); @@ -53,11 +53,11 @@ fn append_row_wrong_layout() { fn append_row_error() { let mut a = Array::zeros((3, 4)); - assert_eq!(a.try_append_row(aview1(&[1.])), + assert_eq!(a.append_row(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); - assert_eq!(a.try_append_column(aview1(&[1.])), + assert_eq!(a.append_column(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); - assert_eq!(a.try_append_column(aview1(&[1., 2., 3.])), + assert_eq!(a.append_column(aview1(&[1., 2., 3.])), Ok(())); assert_eq!(a.t(), array![[0., 0., 0.], @@ -70,8 +70,8 @@ fn append_row_error() { #[test] fn append_row_existing() { let mut a = Array::zeros((1, 4)); - a.try_append_row(aview1(&[0., 1., 2., 3.])).unwrap(); - a.try_append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + a.append_row(aview1(&[0., 1., 2., 3.])).unwrap(); + a.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); assert_eq!(a.shape(), &[3, 4]); assert_eq!(a, @@ -79,11 +79,11 @@ fn append_row_existing() { [0., 1., 2., 3.], [4., 5., 6., 7.]]); - assert_eq!(a.try_append_row(aview1(&[1.])), + assert_eq!(a.append_row(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); - assert_eq!(a.try_append_column(aview1(&[1.])), + assert_eq!(a.append_column(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); - assert_eq!(a.try_append_column(aview1(&[1., 2., 3.])), + assert_eq!(a.append_column(aview1(&[1., 2., 3.])), Ok(())); assert_eq!(a, array![[0., 0., 0., 0., 1.], @@ -95,12 +95,12 @@ fn append_row_existing() { fn append_row_col_len_1() { // Test appending 1 row and then cols from shape 1 x 1 let mut a = Array::zeros((1, 1)); - a.try_append_row(aview1(&[1.])).unwrap(); // shape 2 x 1 - a.try_append_column(aview1(&[2., 3.])).unwrap(); // shape 2 x 2 - assert_eq!(a.try_append_row(aview1(&[1.])), + a.append_row(aview1(&[1.])).unwrap(); // shape 2 x 1 + a.append_column(aview1(&[2., 3.])).unwrap(); // shape 2 x 2 + assert_eq!(a.append_row(aview1(&[1.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleShape))); - //assert_eq!(a.try_append_row(aview1(&[1., 2.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); - a.try_append_column(aview1(&[4., 5.])).unwrap(); // shape 2 x 3 + //assert_eq!(a.append_row(aview1(&[1., 2.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); + a.append_column(aview1(&[4., 5.])).unwrap(); // shape 2 x 3 assert_eq!(a.shape(), &[2, 3]); assert_eq!(a, @@ -111,8 +111,8 @@ fn append_row_col_len_1() { #[test] fn append_column() { let mut a = Array::zeros((4, 0)); - a.try_append_column(aview1(&[0., 1., 2., 3.])).unwrap(); - a.try_append_column(aview1(&[4., 5., 6., 7.])).unwrap(); + a.append_column(aview1(&[0., 1., 2., 3.])).unwrap(); + a.append_column(aview1(&[4., 5., 6., 7.])).unwrap(); assert_eq!(a.shape(), &[4, 2]); assert_eq!(a.t(), @@ -123,18 +123,18 @@ fn append_column() { #[test] fn append_array1() { let mut a = Array::zeros((0, 4)); - a.try_append_array(Axis(0), aview2(&[[0., 1., 2., 3.]])).unwrap(); + a.append(Axis(0), aview2(&[[0., 1., 2., 3.]])).unwrap(); println!("{:?}", a); - a.try_append_array(Axis(0), aview2(&[[4., 5., 6., 7.]])).unwrap(); + a.append(Axis(0), aview2(&[[4., 5., 6., 7.]])).unwrap(); println!("{:?}", a); - //a.try_append_column(aview1(&[4., 5., 6., 7.])).unwrap(); + //a.append_column(aview1(&[4., 5., 6., 7.])).unwrap(); //assert_eq!(a.shape(), &[4, 2]); assert_eq!(a, array![[0., 1., 2., 3.], [4., 5., 6., 7.]]); - a.try_append_array(Axis(0), aview2(&[[5., 5., 4., 4.], [3., 3., 2., 2.]])).unwrap(); + a.append(Axis(0), aview2(&[[5., 5., 4., 4.], [3., 3., 2., 2.]])).unwrap(); println!("{:?}", a); assert_eq!(a, array![[0., 1., 2., 3.], @@ -146,26 +146,26 @@ fn append_array1() { #[test] fn append_array_3d() { let mut a = Array::zeros((0, 2, 2)); - a.try_append_array(Axis(0), array![[[0, 1], [2, 3]]].view()).unwrap(); + a.append(Axis(0), array![[[0, 1], [2, 3]]].view()).unwrap(); println!("{:?}", a); let aa = array![[[51, 52], [53, 54]], [[55, 56], [57, 58]]]; let av = aa.view(); println!("Send {:?} to append", av); - a.try_append_array(Axis(0), av.clone()).unwrap(); + a.append(Axis(0), av.clone()).unwrap(); a.swap_axes(0, 1); let aa = array![[[71, 72], [73, 74]], [[75, 76], [77, 78]]]; let mut av = aa.view(); av.swap_axes(0, 1); println!("Send {:?} to append", av); - a.try_append_array(Axis(1), av.clone()).unwrap(); + a.append(Axis(1), av.clone()).unwrap(); println!("{:?}", a); let aa = array![[[81, 82], [83, 84]], [[85, 86], [87, 88]]]; let mut av = aa.view(); av.swap_axes(0, 1); println!("Send {:?} to append", av); - a.try_append_array(Axis(1), av).unwrap(); + a.append(Axis(1), av).unwrap(); println!("{:?}", a); assert_eq!(a, array![[[0, 1], @@ -190,9 +190,9 @@ fn test_append_2d() { let mut a = Array::zeros((0, 4)); let ones = ArrayView::from(&[1.; 12]).into_shape((3, 4)).unwrap(); let zeros = ArrayView::from(&[0.; 8]).into_shape((2, 4)).unwrap(); - a.try_append_array(Axis(0), ones).unwrap(); - a.try_append_array(Axis(0), zeros).unwrap(); - a.try_append_array(Axis(0), ones).unwrap(); + a.append(Axis(0), ones).unwrap(); + a.append(Axis(0), zeros).unwrap(); + a.append(Axis(0), ones).unwrap(); println!("{:?}", a); assert_eq!(a.shape(), &[8, 4]); for (i, row) in a.rows().into_iter().enumerate() { @@ -204,9 +204,9 @@ fn test_append_2d() { a = a.reversed_axes(); let ones = ones.reversed_axes(); let zeros = zeros.reversed_axes(); - a.try_append_array(Axis(1), ones).unwrap(); - a.try_append_array(Axis(1), zeros).unwrap(); - a.try_append_array(Axis(1), ones).unwrap(); + a.append(Axis(1), ones).unwrap(); + a.append(Axis(1), zeros).unwrap(); + a.append(Axis(1), ones).unwrap(); println!("{:?}", a); assert_eq!(a.shape(), &[4, 8]); @@ -220,16 +220,16 @@ fn test_append_2d() { fn test_append_middle_axis() { // ensure we can append to Axis(1) by letting it become outermost let mut a = Array::::zeros((3, 0, 2)); - a.try_append_array(Axis(1), Array::from_iter(0..12).into_shape((3, 2, 2)).unwrap().view()).unwrap(); + a.append(Axis(1), Array::from_iter(0..12).into_shape((3, 2, 2)).unwrap().view()).unwrap(); println!("{:?}", a); - a.try_append_array(Axis(1), Array::from_iter(12..24).into_shape((3, 2, 2)).unwrap().view()).unwrap(); + a.append(Axis(1), Array::from_iter(12..24).into_shape((3, 2, 2)).unwrap().view()).unwrap(); println!("{:?}", a); // ensure we can append to Axis(1) by letting it become outermost let mut a = Array::::zeros((3, 1, 2)); - a.try_append_array(Axis(1), Array::from_iter(0..12).into_shape((3, 2, 2)).unwrap().view()).unwrap(); + a.append(Axis(1), Array::from_iter(0..12).into_shape((3, 2, 2)).unwrap().view()).unwrap(); println!("{:?}", a); - a.try_append_array(Axis(1), Array::from_iter(12..24).into_shape((3, 2, 2)).unwrap().view()).unwrap(); + a.append(Axis(1), Array::from_iter(12..24).into_shape((3, 2, 2)).unwrap().view()).unwrap(); println!("{:?}", a); } @@ -237,16 +237,16 @@ fn test_append_middle_axis() { fn test_append_zero_size() { { let mut a = Array::::zeros((0, 0)); - a.try_append_array(Axis(0), aview2(&[[]])).unwrap(); - a.try_append_array(Axis(0), aview2(&[[]])).unwrap(); + a.append(Axis(0), aview2(&[[]])).unwrap(); + a.append(Axis(0), aview2(&[[]])).unwrap(); assert_eq!(a.len(), 0); assert_eq!(a.shape(), &[2, 0]); } { let mut a = Array::::zeros((0, 0)); - a.try_append_array(Axis(1), ArrayView::from(&[]).into_shape((0, 1)).unwrap()).unwrap(); - a.try_append_array(Axis(1), ArrayView::from(&[]).into_shape((0, 1)).unwrap()).unwrap(); + a.append(Axis(1), ArrayView::from(&[]).into_shape((0, 1)).unwrap()).unwrap(); + a.append(Axis(1), ArrayView::from(&[]).into_shape((0, 1)).unwrap()).unwrap(); assert_eq!(a.len(), 0); assert_eq!(a.shape(), &[0, 2]); } From 7d522b9408e643c226444ea4a5ffa47774c3a404 Mon Sep 17 00:00:00 2001 From: bluss Date: Wed, 7 Apr 2021 23:37:56 +0200 Subject: [PATCH 20/30] append: Add more append tests for negative stride arrays --- tests/append.rs | 98 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 96 insertions(+), 2 deletions(-) diff --git a/tests/append.rs b/tests/append.rs index 0e24a744d..a67c4e5de 100644 --- a/tests/append.rs +++ b/tests/append.rs @@ -31,11 +31,19 @@ fn append_row_wrong_layout() { a.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); assert_eq!(a.shape(), &[2, 4]); - //assert_eq!(a.append_column(aview1(&[1., 2.])), Err(ShapeError::from_kind(ErrorKind::IncompatibleLayout))); - assert_eq!(a, array![[0., 1., 2., 3.], [4., 5., 6., 7.]]); + assert_eq!(a.strides(), &[4, 1]); + + // Changing the memory layout to fit the next append + let mut a2 = a.clone(); + a2.append_column(aview1(&[1., 2.])).unwrap(); + assert_eq!(a2, + array![[0., 1., 2., 3., 1.], + [4., 5., 6., 7., 2.]]); + assert_eq!(a2.strides(), &[1, 2]); + // Clone the array @@ -49,6 +57,92 @@ fn append_row_wrong_layout() { [4., 5., 6., 7., 2.]]); } +#[test] +fn append_row_neg_stride_1() { + let mut a = Array::zeros((0, 4)); + a.append_row(aview1(&[0., 1., 2., 3.])).unwrap(); + a.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a.shape(), &[2, 4]); + + assert_eq!(a, + array![[0., 1., 2., 3.], + [4., 5., 6., 7.]]); + assert_eq!(a.strides(), &[4, 1]); + + a.invert_axis(Axis(0)); + + // Changing the memory layout to fit the next append + let mut a2 = a.clone(); + println!("a = {:?}", a); + println!("a2 = {:?}", a2); + a2.append_column(aview1(&[1., 2.])).unwrap(); + assert_eq!(a2, + array![[4., 5., 6., 7., 1.], + [0., 1., 2., 3., 2.]]); + assert_eq!(a2.strides(), &[1, 2]); + + a.invert_axis(Axis(1)); + let mut a3 = a.clone(); + a3.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a3, + array![[7., 6., 5., 4.], + [3., 2., 1., 0.], + [4., 5., 6., 7.]]); + assert_eq!(a3.strides(), &[4, 1]); + + a.invert_axis(Axis(0)); + let mut a4 = a.clone(); + a4.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a4, + array![[3., 2., 1., 0.], + [7., 6., 5., 4.], + [4., 5., 6., 7.]]); + assert_eq!(a4.strides(), &[4, -1]); +} + +#[test] +fn append_row_neg_stride_2() { + let mut a = Array::zeros((0, 4)); + a.append_row(aview1(&[0., 1., 2., 3.])).unwrap(); + a.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a.shape(), &[2, 4]); + + assert_eq!(a, + array![[0., 1., 2., 3.], + [4., 5., 6., 7.]]); + assert_eq!(a.strides(), &[4, 1]); + + a.invert_axis(Axis(1)); + + // Changing the memory layout to fit the next append + let mut a2 = a.clone(); + println!("a = {:?}", a); + println!("a2 = {:?}", a2); + a2.append_column(aview1(&[1., 2.])).unwrap(); + assert_eq!(a2, + array![[3., 2., 1., 0., 1.], + [7., 6., 5., 4., 2.]]); + assert_eq!(a2.strides(), &[1, 2]); + + a.invert_axis(Axis(0)); + let mut a3 = a.clone(); + a3.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a3, + array![[7., 6., 5., 4.], + [3., 2., 1., 0.], + [4., 5., 6., 7.]]); + assert_eq!(a3.strides(), &[4, 1]); + + a.invert_axis(Axis(1)); + let mut a4 = a.clone(); + a4.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a4, + array![[4., 5., 6., 7.], + [0., 1., 2., 3.], + [4., 5., 6., 7.]]); + assert_eq!(a4.strides(), &[4, 1]); +} + #[test] fn append_row_error() { let mut a = Array::zeros((3, 4)); From d4819ccb3f9ab9b6366570b43ed964d306243e94 Mon Sep 17 00:00:00 2001 From: bluss Date: Wed, 7 Apr 2021 19:11:43 +0200 Subject: [PATCH 21/30] zip: Make sure Layout::tendency is inlinable This method showed up in profiling because it wasn't inlinable. --- src/layout/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/layout/mod.rs b/src/layout/mod.rs index d4271dd77..e7434fbc1 100644 --- a/src/layout/mod.rs +++ b/src/layout/mod.rs @@ -58,6 +58,7 @@ impl Layout { /// A simple "score" method which scores positive for preferring C-order, negative for F-order /// Subject to change when we can describe other layouts + #[inline] pub(crate) fn tendency(self) -> i32 { (self.is(CORDER) as i32 - self.is(FORDER) as i32) + (self.is(CPREFER) as i32 - self.is(FPREFER) as i32) From f2fd6c244f7c62b813b8d0897c95b08cbd11bd72 Mon Sep 17 00:00:00 2001 From: bluss Date: Wed, 7 Apr 2021 19:13:50 +0200 Subject: [PATCH 22/30] zip: Add Zip::and_unchecked and use to skip redundant dimension check This speeds up the benches/append.rs benchmarks by 5-10%. --- src/impl_owned_array.rs | 5 ++++- src/zip/mod.rs | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 319e310a4..597c2d115 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -540,7 +540,10 @@ impl Array data: &mut self.data, }; - Zip::from(tail_view).and(array) + + // Safety: tail_view is constructed to have the same shape as array + Zip::from(tail_view) + .and_unchecked(array) .debug_assert_c_order() .for_each(|to, from| { to.write(from.clone()); diff --git a/src/zip/mod.rs b/src/zip/mod.rs index ffc6a710a..18d07ddfd 100644 --- a/src/zip/mod.rs +++ b/src/zip/mod.rs @@ -679,6 +679,26 @@ macro_rules! map_impl { self.build_and(part) } + /// Include the producer `p` in the Zip. + /// + /// ## Safety + /// + /// The caller must ensure that the producer's shape is equal to the Zip's shape. + /// Uses assertions when debug assertions are enabled. + #[allow(unused)] + pub(crate) unsafe fn and_unchecked

(self, p: P) -> Zip<($($p,)* P::Output, ), D> + where P: IntoNdProducer, + { + #[cfg(debug_assertions)] + { + self.and(p) + } + #[cfg(not(debug_assertions))] + { + self.build_and(p.into_producer()) + } + } + /// Include the producer `p` in the Zip, broadcasting if needed. /// /// If their shapes disagree, `rhs` is broadcast to the shape of `self`. From c1a5cebea968367ba6032afd2f5fc1dab3ad0e13 Mon Sep 17 00:00:00 2001 From: bluss Date: Thu, 8 Apr 2021 20:27:22 +0200 Subject: [PATCH 23/30] stacking: Improve .select() with special case for 1D arrays The 1D case is a simpler gather operation since we only select 1 element per index. Special-case it. The performance increase for this benchmark is that the benchmark runtime changes by -92% with this change. --- benches/append.rs | 11 +++++++++++ src/impl_methods.rs | 37 ++++++++++++++++++++++++++++--------- src/lib.rs | 1 + tests/array.rs | 13 +++++++++++++ 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/benches/append.rs b/benches/append.rs index 5502fa062..1a911a278 100644 --- a/benches/append.rs +++ b/benches/append.rs @@ -22,3 +22,14 @@ fn select_axis1(bench: &mut Bencher) { a.select(Axis(1), &selectable) }); } + +#[bench] +fn select_1d(bench: &mut Bencher) { + let a = Array::::zeros(1024); + let mut selectable = (0..a.len()).step_by(17).collect::>(); + selectable.extend(selectable.clone().iter().rev()); + + bench.iter(|| { + a.select(Axis(0), &selectable) + }); +} diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 3e893fcd8..fccce3179 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -877,16 +877,35 @@ where S: Data, D: RemoveAxis, { - let mut subs = vec![self.view(); indices.len()]; - for (&i, sub) in zip(indices, &mut subs[..]) { - sub.collapse_axis(axis, i); - } - if subs.is_empty() { - let mut dim = self.raw_dim(); - dim.set_axis(axis, 0); - unsafe { Array::from_shape_vec_unchecked(dim, vec![]) } + if self.ndim() == 1 { + // using .len_of(axis) means that we check if `axis` is in bounds too. + let axis_len = self.len_of(axis); + // bounds check the indices first + if let Some(max_index) = indices.iter().cloned().max() { + if max_index >= axis_len { + panic!("ndarray: index {} is out of bounds in array of len {}", + max_index, self.len_of(axis)); + } + } // else: indices empty is ok + let view = self.view().into_dimensionality::().unwrap(); + Array::from_iter(indices.iter().map(move |&index| { + // Safety: bounds checked indexes + unsafe { + view.uget(index).clone() + } + })).into_dimensionality::().unwrap() } else { - concatenate(axis, &subs).unwrap() + let mut subs = vec![self.view(); indices.len()]; + for (&i, sub) in zip(indices, &mut subs[..]) { + sub.collapse_axis(axis, i); + } + if subs.is_empty() { + let mut dim = self.raw_dim(); + dim.set_axis(axis, 0); + unsafe { Array::from_shape_vec_unchecked(dim, vec![]) } + } else { + concatenate(axis, &subs).unwrap() + } } } diff --git a/src/lib.rs b/src/lib.rs index 6a3f0a76b..06aca0150 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,6 +13,7 @@ clippy::unreadable_literal, clippy::manual_map, // is not an error clippy::while_let_on_iterator, // is not an error + clippy::from_iter_instead_of_collect, // using from_iter is good style )] #![cfg_attr(not(feature = "std"), no_std)] diff --git a/tests/array.rs b/tests/array.rs index 38d2711aa..976824dfe 100644 --- a/tests/array.rs +++ b/tests/array.rs @@ -709,6 +709,19 @@ fn test_select() { assert_abs_diff_eq!(c, c_target); } +#[test] +fn test_select_1d() { + let x = arr1(&[0, 1, 2, 3, 4, 5, 6]); + let r1 = x.select(Axis(0), &[1, 3, 4, 2, 2, 5]); + assert_eq!(r1, arr1(&[1, 3, 4, 2, 2, 5])); + // select nothing + let r2 = x.select(Axis(0), &[]); + assert_eq!(r2, arr1(&[])); + // select nothing from empty + let r3 = r2.select(Axis(0), &[]); + assert_eq!(r3, arr1(&[])); +} + #[test] fn diag() { let d = arr2(&[[1., 2., 3.0f32]]).into_diag(); From 8d44bbc49d712dde731d0227c0852aea2cb73512 Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 11 Apr 2021 15:46:51 +0200 Subject: [PATCH 24/30] append: Edits and clarifications in append comments Co-authored-by: Jim Turner --- src/impl_owned_array.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 597c2d115..f0689c146 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -86,12 +86,12 @@ impl Array { /// Appending automatically changes memory layout of the array so that it is appended to /// along the "growing axis". /// - /// Ensure appending is efficient by for example starting appending an empty array and + /// Ensure appending is efficient by, for example, appending to an empty array and then /// always appending along the same axis. For rows, ndarray's default layout is efficient for /// appending. /// /// Notice that an empty array (where it has an axis of length zero) is the simplest starting - /// point. The amortized average complexity of the append is O(m) where *m* is the length of + /// point. When repeatedly appending to a single axis, the amortized average complexity of each append is O(m), where *m* is the length of /// the row. /// /// ```rust @@ -123,12 +123,12 @@ impl Array { /// Appending automatically changes memory layout of the array so that it is appended to /// along the "growing axis". /// - /// Ensure appending is efficient by for example starting appending an empty array and + /// Ensure appending is efficient by, for example, appending to an empty array and then /// always appending along the same axis. For columns, column major ("F") memory layout is /// efficient for appending. /// /// Notice that an empty array (where it has an axis of length zero) is the simplest starting - /// point. The amortized average complexity of the append is O(m) where *m* is the length of + /// point. When repeatedly appending to a single axis, the amortized average complexity of each append is O(m), where *m* is the length of /// the row. /// /// ```rust @@ -222,7 +222,7 @@ impl Array #[cold] fn drop_unreachable_elements_slow(mut self) -> OwnedRepr { // "deconstruct" self; the owned repr releases ownership of all elements and we - // and carry on with raw view methods + // carry on with raw view methods let self_len = self.len(); let data_len = self.data.len(); let data_ptr = self.data.as_nonnull_mut().as_ptr(); @@ -230,7 +230,8 @@ impl Array let mut self_; unsafe { - // Safety: self.data releases ownership of the elements + // Safety: self.data releases ownership of the elements. Any panics below this point + // will result in leaking elements instead of double drops. self_ = self.raw_view_mut(); self.data.set_len(0); } @@ -430,7 +431,7 @@ impl Array } } - // array must be be "full" (have no exterior holes) + // array must be be "full" (contiguous and have no exterior holes) if self.len() != self.data.len() { incompatible_layout = true; } @@ -520,7 +521,7 @@ impl Array tail_view.shape(), tail_view.strides()); } - // Keep track of currently filled lenght of `self.data` and update it + // Keep track of currently filled length of `self.data` and update it // on scope exit (panic or loop finish). struct SetLenOnDrop<'a, A: 'a> { len: usize, @@ -646,4 +647,3 @@ where } } } - From b103515baa57945615536eb98b2369a136bf9645 Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 11 Apr 2021 15:47:34 +0200 Subject: [PATCH 25/30] stacking: Update concatenate test Co-authored-by: Jim Turner --- tests/stacking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/stacking.rs b/tests/stacking.rs index 2c223fd66..0c4e79c79 100644 --- a/tests/stacking.rs +++ b/tests/stacking.rs @@ -20,7 +20,7 @@ fn concatenating() { [2., 9.]])); let d = concatenate![Axis(0), a.row(0).insert_axis(Axis(1)), aview1(&[9., 9.]).insert_axis(Axis(1))]; - assert_eq!(d.t(), aview2(&[[2., 2., 9., 9.]])); + assert_eq!(d, aview2(&[[2.], [2.], [9.], [9.]])); let res = ndarray::concatenate(Axis(1), &[a.view(), c.view()]); assert_eq!(res.unwrap_err().kind(), ErrorKind::IncompatibleShape); From 59013cdc7a5a22f0911492ef637231b94c4890da Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 11 Apr 2021 21:41:36 +0200 Subject: [PATCH 26/30] move_into: Implement inner-dimension optimization in move-into If the innermost dimension is contiguous, we can skip it in one go, and save some work in the dropping loop in move_into. Co-authored-by: Jim Turner --- src/impl_owned_array.rs | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index f0689c146..f0ae42c71 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -255,10 +255,6 @@ impl Array debug_assert!(data_ptr <= array_memory_head_ptr); debug_assert!(array_memory_head_ptr <= data_end_ptr); - // iter is a raw pointer iterator traversing self_ in its standard order - let mut iter = Baseiter::new(self_.ptr.as_ptr(), self_.dim, self_.strides); - let mut dropped_elements = 0; - // The idea is simply this: the iterator will yield the elements of self_ in // increasing address order. // @@ -267,6 +263,25 @@ impl Array // // We have to drop elements in the range from `data_ptr` until (not including) // `data_end_ptr`, except those that are produced by `iter`. + + // As an optimization, the innermost axis is removed if it has stride 1, because + // we then have a long stretch of contiguous elements we can skip as one. + let inner_lane_len; + if self_.ndim() > 1 && self_.strides.last_elem() == 1 { + self_.dim.slice_mut().rotate_right(1); + self_.strides.slice_mut().rotate_right(1); + inner_lane_len = self_.dim[0]; + self_.dim[0] = 1; + self_.strides[0] = 1; + } else { + inner_lane_len = 1; + } + + // iter is a raw pointer iterator traversing the array in memory order now with the + // sorted axes. + let mut iter = Baseiter::new(self_.ptr.as_ptr(), self_.dim, self_.strides); + let mut dropped_elements = 0; + let mut last_ptr = data_ptr; while let Some(elem_ptr) = iter.next() { @@ -278,8 +293,8 @@ impl Array last_ptr = last_ptr.add(1); dropped_elements += 1; } - // Next interval will continue one past the current element - last_ptr = elem_ptr.add(1); + // Next interval will continue one past the current lane + last_ptr = elem_ptr.add(inner_lane_len); } while last_ptr < data_end_ptr { From c9dd195b69c6f9c203594b79828a6eb30d2c5fe5 Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 11 Apr 2021 21:52:16 +0200 Subject: [PATCH 27/30] append: Add comment explaining the SetLenOnDrop optimization Benchmarks (append benchmarks) show that this is a worthwhile optimization, and that plain self.data.set_len() in the loop performs worse. --- src/impl_owned_array.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index f0ae42c71..94f610b15 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -537,7 +537,9 @@ impl Array } // Keep track of currently filled length of `self.data` and update it - // on scope exit (panic or loop finish). + // on scope exit (panic or loop finish). This "indirect" way to + // write the length is used to help the compiler, the len store to self.data may + // otherwise be mistaken to alias with other stores in the loop. struct SetLenOnDrop<'a, A: 'a> { len: usize, data: &'a mut OwnedRepr, From 3f59442e60e208e2d69a6453604bcd230ba1556b Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 11 Apr 2021 22:12:49 +0200 Subject: [PATCH 28/30] append: Use standard order for non-growing axes in append Imagine that the array-to-append has length 1 along the growing axis. With this memory layout, the resulting assignment when copying the array elements into `self`, can often be a contiguous traversal. Co-authored-by: Jim Turner --- src/impl_owned_array.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 94f610b15..208eee019 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -327,13 +327,14 @@ impl Array // The array will be created with 0 (C) or ndim-1 (F) as the biggest stride // axis. Rearrange the shape so that `growing_axis` is the biggest stride axis // afterwards. - let prefer_f_layout = growing_axis == Axis(ndim - 1); - if !prefer_f_layout { - dim.slice_mut().swap(0, growing_axis.index()); - } - let mut new_array = Self::uninit(dim.set_f(prefer_f_layout)); - if !prefer_f_layout { - new_array.swap_axes(0, growing_axis.index()); + let mut new_array; + if growing_axis == Axis(ndim - 1) { + new_array = Self::uninit(dim.f()); + } else { + dim.slice_mut()[..=growing_axis.index()].rotate_right(1); + new_array = Self::uninit(dim); + new_array.dim.slice_mut()[..=growing_axis.index()].rotate_left(1); + new_array.strides.slice_mut()[..=growing_axis.index()].rotate_left(1); } // self -> old_self. @@ -467,11 +468,13 @@ impl Array // Axis n - 1 is outermost axis res_dim.fortran_strides() } else { - // Default with modification - res_dim.slice_mut().swap(0, axis.index()); + // standard axis order except for the growing axis; + // anticipates that it's likely that `array` has standard order apart from the + // growing axis. + res_dim.slice_mut()[..=axis.index()].rotate_right(1); let mut strides = res_dim.default_strides(); - res_dim.slice_mut().swap(0, axis.index()); - strides.slice_mut().swap(0, axis.index()); + res_dim.slice_mut()[..=axis.index()].rotate_left(1); + strides.slice_mut()[..=axis.index()].rotate_left(1); strides } } else if current_axis_len == 1 { From 1206bd6aacdeb9334202568febcd8261bf16b0dc Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 11 Apr 2021 22:22:49 +0200 Subject: [PATCH 29/30] append: Use positive stride and ignore stride of len 1 axes Fix two bugs that Jim found in how we calculate the new stride for the growing axis. Tests by Jim Turner. Co-authored-by: Jim Turner --- src/impl_owned_array.rs | 8 +++++--- tests/append.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 208eee019..bef2aa60d 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -480,9 +480,11 @@ impl Array } else if current_axis_len == 1 { // This is the outermost/longest stride axis; so we find the max across the other axes let new_stride = self.axes().fold(1, |acc, ax| { - if ax.axis == axis { acc } else { - let this_ax = ax.len as isize * ax.stride; - if this_ax.abs() > acc { this_ax } else { acc } + if ax.axis == axis || ax.len <= 1 { + acc + } else { + let this_ax = ax.len as isize * ax.stride.abs(); + if this_ax > acc { this_ax } else { acc } } }); let mut strides = self.strides.clone(); diff --git a/tests/append.rs b/tests/append.rs index a67c4e5de..6306b2ecf 100644 --- a/tests/append.rs +++ b/tests/append.rs @@ -345,3 +345,34 @@ fn test_append_zero_size() { assert_eq!(a.shape(), &[0, 2]); } } + +#[test] +fn append_row_neg_stride_3() { + let mut a = Array::zeros((0, 4)); + a.append_row(aview1(&[0., 1., 2., 3.])).unwrap(); + a.invert_axis(Axis(1)); + a.append_row(aview1(&[4., 5., 6., 7.])).unwrap(); + assert_eq!(a.shape(), &[2, 4]); + assert_eq!(a, array![[3., 2., 1., 0.], [4., 5., 6., 7.]]); + assert_eq!(a.strides(), &[4, -1]); +} + +#[test] +fn append_row_ignore_strides_length_one_axes() { + let strides = &[0, 1, 10, 20]; + for invert in &[vec![], vec![0], vec![1], vec![0, 1]] { + for &stride0 in strides { + for &stride1 in strides { + let mut a = + Array::from_shape_vec([1, 1].strides([stride0, stride1]), vec![0.]).unwrap(); + for &ax in invert { + a.invert_axis(Axis(ax)); + } + a.append_row(aview1(&[1.])).unwrap(); + assert_eq!(a.shape(), &[2, 1]); + assert_eq!(a, array![[0.], [1.]]); + assert_eq!(a.stride_of(Axis(0)), 1); + } + } + } +} From d946360ada25b1218be1a61afb67fd0b3818d46f Mon Sep 17 00:00:00 2001 From: bluss Date: Tue, 13 Apr 2021 21:07:56 +0200 Subject: [PATCH 30/30] append: performance optimize stride check This .max_by_key() call showed up in profiling, and this change improves the select_axis0 benchmark by reducing runtime by 15% --- src/impl_owned_array.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index bef2aa60d..a795a354a 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -438,12 +438,19 @@ impl Array // array must be empty or have `axis` as the outermost (longest stride) axis if !self_is_empty && current_axis_len > 1 { // `axis` must be max stride axis or equal to its stride - let max_axis = self.axes().max_by_key(|ax| ax.stride.abs()).unwrap(); - if max_axis.axis != axis && max_axis.stride.abs() > self.stride_of(axis) { - incompatible_layout = true; - } - if self.stride_of(axis) < 0 { + let axis_stride = self.stride_of(axis); + if axis_stride < 0 { incompatible_layout = true; + } else { + for ax in self.axes() { + if ax.axis == axis { + continue; + } + if ax.len > 1 && ax.stride.abs() > axis_stride { + incompatible_layout = true; + break; + } + } } }