From 2ec6bba645b1410f0a2f7170f51cdefb8e3a778f Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 4 Apr 2021 03:24:17 +0200 Subject: [PATCH 1/6] FEAT: Add method .shift_remove_index --- src/impl_methods.rs | 25 +++++++++++++++++++++++++ tests/array.rs | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/impl_methods.rs b/src/impl_methods.rs index efea69c4c..5274f86a4 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -2444,6 +2444,31 @@ where } } + /// Remove the `index`th elements along `axis` and shift down elements from higher indexes. + /// + /// Decreases the length of `axis` by one. + /// + /// ***Panics** if `axis` or `index` is out of bounds. + pub fn shift_remove_index(&mut self, axis: Axis, index: usize) + where + S: DataOwned + DataMut, + { + let (_, mut tail) = self.view_mut().split_at(axis, index); + // shift elements to the back + // use swap to keep all elements initialized (as required by owned storage) + Zip::from(tail.lanes_mut(axis)).for_each(|mut lane| { + let mut lane_iter = lane.iter_mut(); + let mut dst = if let Some(dst) = lane_iter.next() { dst } else { return }; + + for elt in lane_iter { + std::mem::swap(dst, elt); + dst = elt; + } + }); + // then slice the axis in place to cut out the removed final element + self.slice_axis_inplace(axis, Slice::new(0, Some(-1), 1)); + } + /// Iterates over pairs of consecutive elements along the axis. /// /// The first argument to the closure is an element, and the second diff --git a/tests/array.rs b/tests/array.rs index 9e45d161f..108e3b1ab 100644 --- a/tests/array.rs +++ b/tests/array.rs @@ -2397,3 +2397,45 @@ mod array_cow_tests { }); } } + +#[test] +fn test_shift_remove() { + let mut a = arr2(&[[1, 2, 3], + [4, 5, 6], + [7, 8, 9], + [10,11,12]]); + a.shift_remove_index(Axis(0), 1); + a.shift_remove_index(Axis(1), 2); + assert_eq!(a.shape(), &[3, 2]); + assert_eq!(a, + array![[1, 2], + [7, 8], + [10,11]]); + + let mut a = arr2(&[[1, 2, 3], + [4, 5, 6], + [7, 8, 9], + [10,11,12]]); + a.invert_axis(Axis(0)); + a.shift_remove_index(Axis(0), 1); + a.shift_remove_index(Axis(1), 2); + assert_eq!(a.shape(), &[3, 2]); + assert_eq!(a, + array![[10,11], + [4, 5], + [1, 2]]); + + a.shift_remove_index(Axis(1), 1); + + assert_eq!(a.shape(), &[3, 1]); + assert_eq!(a, + array![[10], + [4], + [1]]); + a.shift_remove_index(Axis(1), 0); + assert_eq!(a.shape(), &[3, 0]); + assert_eq!(a, + array![[], + [], + []]); +} From 6e710b79956070a047e1c90a24b983d06e9c3c53 Mon Sep 17 00:00:00 2001 From: bluss Date: Sun, 4 Apr 2021 03:35:10 +0200 Subject: [PATCH 2/6] FEAT: Optimize shift_remove_index using MaybeUninit --- src/impl_methods.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 5274f86a4..5fb7b9c98 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -6,7 +6,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use std::mem::{size_of, ManuallyDrop}; +use std::mem::{size_of, ManuallyDrop, MaybeUninit}; use alloc::slice; use alloc::vec; use alloc::vec::Vec; @@ -2460,9 +2460,22 @@ where let mut lane_iter = lane.iter_mut(); let mut dst = if let Some(dst) = lane_iter.next() { dst } else { return }; - for elt in lane_iter { - std::mem::swap(dst, elt); - dst = elt; + // Logically we do a circular swap here, all elements in a chain + // Using MaybeUninit to avoid unecessary writes in the safe swap solution + // + // for elt in lane_iter { + // std::mem::swap(dst, elt); + // dst = elt; + // } + // + let mut slot = MaybeUninit::::uninit(); + unsafe { + slot.as_mut_ptr().copy_from_nonoverlapping(dst, 1); + for elt in lane_iter { + (dst as *mut A).copy_from_nonoverlapping(elt, 1); + dst = elt; + } + (dst as *mut A).copy_from_nonoverlapping(slot.as_ptr(), 1); } }); // then slice the axis in place to cut out the removed final element From c568124d4da852960db7a49cf57699cc91ed1b2e Mon Sep 17 00:00:00 2001 From: bluss Date: Mon, 5 Apr 2021 14:52:38 +0200 Subject: [PATCH 3/6] API: Name and comment changes for remove_index Co-authored-by: Jim Turner --- src/impl_methods.rs | 6 +++--- tests/array.rs | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 5fb7b9c98..8d4c8435c 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -2449,13 +2449,13 @@ where /// Decreases the length of `axis` by one. /// /// ***Panics** if `axis` or `index` is out of bounds. - pub fn shift_remove_index(&mut self, axis: Axis, index: usize) + pub fn remove_index(&mut self, axis: Axis, index: usize) where S: DataOwned + DataMut, { let (_, mut tail) = self.view_mut().split_at(axis, index); - // shift elements to the back - // use swap to keep all elements initialized (as required by owned storage) + // shift elements to the front + // use swapping to keep all elements initialized (as required by owned storage) Zip::from(tail.lanes_mut(axis)).for_each(|mut lane| { let mut lane_iter = lane.iter_mut(); let mut dst = if let Some(dst) = lane_iter.next() { dst } else { return }; diff --git a/tests/array.rs b/tests/array.rs index 108e3b1ab..8bbc6205c 100644 --- a/tests/array.rs +++ b/tests/array.rs @@ -2399,13 +2399,13 @@ mod array_cow_tests { } #[test] -fn test_shift_remove() { +fn test_remove_index() { let mut a = arr2(&[[1, 2, 3], [4, 5, 6], [7, 8, 9], [10,11,12]]); - a.shift_remove_index(Axis(0), 1); - a.shift_remove_index(Axis(1), 2); + a.remove_index(Axis(0), 1); + a.remove_index(Axis(1), 2); assert_eq!(a.shape(), &[3, 2]); assert_eq!(a, array![[1, 2], @@ -2417,22 +2417,22 @@ fn test_shift_remove() { [7, 8, 9], [10,11,12]]); a.invert_axis(Axis(0)); - a.shift_remove_index(Axis(0), 1); - a.shift_remove_index(Axis(1), 2); + a.remove_index(Axis(0), 1); + a.remove_index(Axis(1), 2); assert_eq!(a.shape(), &[3, 2]); assert_eq!(a, array![[10,11], [4, 5], [1, 2]]); - a.shift_remove_index(Axis(1), 1); + a.remove_index(Axis(1), 1); assert_eq!(a.shape(), &[3, 1]); assert_eq!(a, array![[10], [4], [1]]); - a.shift_remove_index(Axis(1), 0); + a.remove_index(Axis(1), 0); assert_eq!(a.shape(), &[3, 0]); assert_eq!(a, array![[], From 107f51f2cee9de50b086bef521896df624d0d2a8 Mon Sep 17 00:00:00 2001 From: bluss Date: Mon, 5 Apr 2021 15:02:45 +0200 Subject: [PATCH 4/6] FIX: in remove_axis(axis, index) specify we need index < len_of(axis) Removing the index-past-the end does not make sense (it's an ok split index, so the previous code passed through without action). --- src/impl_methods.rs | 10 +++++++++- tests/array.rs | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 8d4c8435c..f4769a13c 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -2446,13 +2446,21 @@ where /// Remove the `index`th elements along `axis` and shift down elements from higher indexes. /// + /// Note that this "removes" the elements by swapping them around to the end of the axis and + /// shortening the length of the axis; the elements are not deinitialized or dropped by this, + /// just moved out of view (this only matters for elements with ownership semantics). It's + /// similar to slicing an owned array in place. + /// /// Decreases the length of `axis` by one. /// - /// ***Panics** if `axis` or `index` is out of bounds. + /// ***Panics*** if `axis` is out of bounds
+ /// ***Panics*** if not `index < self.len_of(axis)`. pub fn remove_index(&mut self, axis: Axis, index: usize) where S: DataOwned + DataMut, { + assert!(index < self.len_of(axis), "index {} must be less than length of Axis({})", + index, axis.index()); let (_, mut tail) = self.view_mut().split_at(axis, index); // shift elements to the front // use swapping to keep all elements initialized (as required by owned storage) diff --git a/tests/array.rs b/tests/array.rs index 8bbc6205c..38d2711aa 100644 --- a/tests/array.rs +++ b/tests/array.rs @@ -2439,3 +2439,37 @@ fn test_remove_index() { [], []]); } + +#[should_panic(expected="must be less")] +#[test] +fn test_remove_index_oob1() { + let mut a = arr2(&[[1, 2, 3], + [4, 5, 6], + [7, 8, 9], + [10,11,12]]); + a.remove_index(Axis(0), 4); +} + +#[should_panic(expected="must be less")] +#[test] +fn test_remove_index_oob2() { + let mut a = array![[10], [4], [1]]; + a.remove_index(Axis(1), 0); + assert_eq!(a.shape(), &[3, 0]); + assert_eq!(a, + array![[], + [], + []]); + a.remove_index(Axis(0), 1); // ok + assert_eq!(a, + array![[], + []]); + a.remove_index(Axis(1), 0); // oob +} + +#[should_panic(expected="index out of bounds")] +#[test] +fn test_remove_index_oob3() { + let mut a = array![[10], [4], [1]]; + a.remove_index(Axis(2), 0); +} From 0d5ea8435bf598cd50b8ed56dc2d453bb31d4f75 Mon Sep 17 00:00:00 2001 From: bluss Date: Mon, 5 Apr 2021 16:48:15 +0200 Subject: [PATCH 5/6] FIX: Add AbortIfPanic guard to remove_index It's a critical section where we can't panic. Mark the reason in the guard and message. --- src/impl_methods.rs | 3 +++ src/lib.rs | 1 + src/low_level_util.rs | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+) create mode 100644 src/low_level_util.rs diff --git a/src/impl_methods.rs b/src/impl_methods.rs index f4769a13c..51f638d3f 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -26,6 +26,7 @@ use crate::dimension::broadcast::co_broadcast; use crate::error::{self, ErrorKind, ShapeError, from_kind}; use crate::math_cell::MathCell; use crate::itertools::zip; +use crate::low_level_util::AbortIfPanic; use crate::zip::{IntoNdProducer, Zip}; use crate::AxisDescription; @@ -2476,6 +2477,7 @@ where // dst = elt; // } // + let guard = AbortIfPanic(&"remove_index: temporarily moving out of owned value"); let mut slot = MaybeUninit::::uninit(); unsafe { slot.as_mut_ptr().copy_from_nonoverlapping(dst, 1); @@ -2485,6 +2487,7 @@ where } (dst as *mut A).copy_from_nonoverlapping(slot.as_ptr(), 1); } + guard.defuse(); }); // then slice the axis in place to cut out the removed final element self.slice_axis_inplace(axis, Slice::new(0, Some(-1), 1)); diff --git a/src/lib.rs b/src/lib.rs index 35b64dce8..762a55637 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -205,6 +205,7 @@ mod shape_builder; mod slice; mod split_at; mod stacking; +mod low_level_util; #[macro_use] mod zip; diff --git a/src/low_level_util.rs b/src/low_level_util.rs new file mode 100644 index 000000000..b61b06f0d --- /dev/null +++ b/src/low_level_util.rs @@ -0,0 +1,40 @@ +// Copyright 2021 bluss and ndarray developers. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + + +/// Guard value that will abort if it is dropped. +/// To defuse, this value must be forgotten before the end of the scope. +/// +/// The string value is added to the message printed if aborting. +#[must_use] +pub(crate) struct AbortIfPanic(pub(crate) &'static &'static str); + +impl AbortIfPanic { + /// Defuse the AbortIfPanic guard. This *must* be done when finished. + #[inline] + pub(crate) fn defuse(self) { + std::mem::forget(self); + } +} + +impl Drop for AbortIfPanic { + // The compiler should be able to remove this, if it can see through that there + // is no panic in the code section. + fn drop(&mut self) { + #[cfg(feature="std")] + { + eprintln!("ndarray: panic in no-panic section, aborting: {}", self.0); + std::process::abort() + } + #[cfg(not(feature="std"))] + { + // no-std uses panic-in-panic (should abort) + panic!("ndarray: panic in no-panic section, bailing out: {}", self.0); + } + } +} From b90f1f96ebe6d7fccc01d95e6a5614550beeacaf Mon Sep 17 00:00:00 2001 From: bluss Date: Fri, 9 Apr 2021 18:29:54 +0200 Subject: [PATCH 6/6] FIX: Factor out rotate1_front function from remove_index --- src/impl_1d.rs | 34 ++++++++++++++++++++++++++++++++++ src/impl_methods.rs | 29 ++--------------------------- 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/impl_1d.rs b/src/impl_1d.rs index 704d8643d..74eaaad21 100644 --- a/src/impl_1d.rs +++ b/src/impl_1d.rs @@ -8,7 +8,10 @@ //! Methods for one-dimensional arrays. use alloc::vec::Vec; +use std::mem::MaybeUninit; + use crate::imp_prelude::*; +use crate::low_level_util::AbortIfPanic; /// # Methods For 1-D Arrays impl ArrayBase @@ -27,4 +30,35 @@ where crate::iterators::to_vec(self.iter().cloned()) } } + + /// Rotate the elements of the array by 1 element towards the front; + /// the former first element becomes the last. + pub(crate) fn rotate1_front(&mut self) + where + S: DataMut, + { + // use swapping to keep all elements initialized (as required by owned storage) + let mut lane_iter = self.iter_mut(); + let mut dst = if let Some(dst) = lane_iter.next() { dst } else { return }; + + // Logically we do a circular swap here, all elements in a chain + // Using MaybeUninit to avoid unecessary writes in the safe swap solution + // + // for elt in lane_iter { + // std::mem::swap(dst, elt); + // dst = elt; + // } + // + let guard = AbortIfPanic(&"rotate1_front: temporarily moving out of owned value"); + let mut slot = MaybeUninit::::uninit(); + unsafe { + slot.as_mut_ptr().copy_from_nonoverlapping(dst, 1); + for elt in lane_iter { + (dst as *mut A).copy_from_nonoverlapping(elt, 1); + dst = elt; + } + (dst as *mut A).copy_from_nonoverlapping(slot.as_ptr(), 1); + } + guard.defuse(); + } } diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 51f638d3f..03ca09d74 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -6,7 +6,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use std::mem::{size_of, ManuallyDrop, MaybeUninit}; +use std::mem::{size_of, ManuallyDrop}; use alloc::slice; use alloc::vec; use alloc::vec::Vec; @@ -26,7 +26,6 @@ use crate::dimension::broadcast::co_broadcast; use crate::error::{self, ErrorKind, ShapeError, from_kind}; use crate::math_cell::MathCell; use crate::itertools::zip; -use crate::low_level_util::AbortIfPanic; use crate::zip::{IntoNdProducer, Zip}; use crate::AxisDescription; @@ -2464,31 +2463,7 @@ where index, axis.index()); let (_, mut tail) = self.view_mut().split_at(axis, index); // shift elements to the front - // use swapping to keep all elements initialized (as required by owned storage) - Zip::from(tail.lanes_mut(axis)).for_each(|mut lane| { - let mut lane_iter = lane.iter_mut(); - let mut dst = if let Some(dst) = lane_iter.next() { dst } else { return }; - - // Logically we do a circular swap here, all elements in a chain - // Using MaybeUninit to avoid unecessary writes in the safe swap solution - // - // for elt in lane_iter { - // std::mem::swap(dst, elt); - // dst = elt; - // } - // - let guard = AbortIfPanic(&"remove_index: temporarily moving out of owned value"); - let mut slot = MaybeUninit::::uninit(); - unsafe { - slot.as_mut_ptr().copy_from_nonoverlapping(dst, 1); - for elt in lane_iter { - (dst as *mut A).copy_from_nonoverlapping(elt, 1); - dst = elt; - } - (dst as *mut A).copy_from_nonoverlapping(slot.as_ptr(), 1); - } - guard.defuse(); - }); + Zip::from(tail.lanes_mut(axis)).for_each(|mut lane| lane.rotate1_front()); // then slice the axis in place to cut out the removed final element self.slice_axis_inplace(axis, Slice::new(0, Some(-1), 1)); }