From 135d555aa9d9dfe8c3570aba9f8f0ce45844a14e Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sat, 8 May 2021 23:14:22 -0400 Subject: [PATCH 1/6] Add offset_to_first_elem method for owned arrays --- src/impl_owned_array.rs | 58 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 63af63917..441ebf4fb 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -61,6 +61,55 @@ impl Array where D: Dimension, { + /// Returns the offset (in units of `A`) from the start of the raw data to + /// the first element, or `None` if the array is empty. + /// + /// In other words, after converting the array to `Vec` with + /// [`.into_raw_vec()`](Self::into_raw_vec), this is the offset from the + /// start of the `Vec` to the first element of the array. + /// + /// ``` + /// use ndarray::{Array2, Axis}; + /// + /// let mut arr: Array2 = Array2::zeros([3, 4]); + /// arr.slice_axis_inplace(Axis(0), (1..).into()); + /// arr[[0, 0]] = 5.; + /// + /// let offset = arr.offset_to_first_elem().unwrap(); + /// assert_eq!(offset, 4); + /// + /// let v = arr.into_raw_vec(); + /// assert_eq!(v[offset], 5.); + /// ``` + /// + /// In the case of zero-sized elements, the offset is somewhat meaningless. + /// For convenience, an offset will be returned such that all indices + /// computed using the offset, shape, and strides will be in-bounds for the + /// `Vec` returned by [`.into_raw_vec()`](Self::into_raw_vec). Note that + /// this offset won't necessarily be the same as the offset for an array of + /// nonzero-sized elements sliced in the same way. + /// + /// ``` + /// use ndarray::{Array2, Axis}; + /// + /// let mut arr: Array2<()> = Array2::from_elem([3, 4], ()); + /// arr.slice_axis_inplace(Axis(0), (1..).into()); + /// let offset = arr.offset_to_first_elem().unwrap(); + /// assert_eq!(offset, 0); + /// ``` + pub fn offset_to_first_elem(&self) -> Option { + if self.is_empty() { + return None; + } + let offset = if std::mem::size_of::() == 0 { + -dimension::offset_from_ptr_to_memory(&self.dim, &self.strides) + } else { + unsafe { self.as_ptr().offset_from(self.data.as_ptr()) } + }; + debug_assert!(offset >= 0); + Some(offset as usize) + } + /// Return a vector of the elements in the array, in the way they are /// stored internally. /// @@ -491,13 +540,8 @@ impl Array unsafe { // grow backing storage and update head ptr - 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); + let data_to_array_offset = self.offset_to_first_elem().unwrap_or(0); + self.ptr = self.data.reserve(len_to_append).add(data_to_array_offset); // clone elements from view to the array now // From 275e83eda75d2317088d82443795446825114a8f Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 9 May 2021 00:09:02 -0400 Subject: [PATCH 2/6] Fix/suppress clippy warnings --- src/arrayformat.rs | 4 +--- src/data_traits.rs | 1 + 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/arrayformat.rs b/src/arrayformat.rs index c1ca352e3..5998a0c1e 100644 --- a/src/arrayformat.rs +++ b/src/arrayformat.rs @@ -50,10 +50,8 @@ impl FormatOptions { self.axis_collapse_limit = std::usize::MAX; self.axis_collapse_limit_next_last = std::usize::MAX; self.axis_collapse_limit_last = std::usize::MAX; - self - } else { - self } + self } /// Axis length collapse limit before ellipsizing, where `axis_rindex` is diff --git a/src/data_traits.rs b/src/data_traits.rs index 7ac63d54e..7d9f15b0a 100644 --- a/src/data_traits.rs +++ b/src/data_traits.rs @@ -252,6 +252,7 @@ unsafe impl Data for OwnedArcRepr { } } + #[allow(clippy::wrong_self_convention)] fn to_shared(self_: &ArrayBase) -> ArrayBase, D> where Self::Elem: Clone, From 6812592dcffe75bb502b043f8284d868cf4084ea Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 9 May 2021 14:57:13 -0400 Subject: [PATCH 3/6] Change offset_from_ptr_to_memory to offset_from_low_addr_ptr_to_logical_ptr --- src/dimension/mod.rs | 22 ++++++++++++---------- src/impl_constructors.rs | 4 ++-- src/impl_methods.rs | 10 +++++----- src/impl_owned_array.rs | 15 +++++++++------ src/impl_views/constructors.rs | 6 +++--- 5 files changed, 31 insertions(+), 26 deletions(-) diff --git a/src/dimension/mod.rs b/src/dimension/mod.rs index f4f46e764..5f597efd1 100644 --- a/src/dimension/mod.rs +++ b/src/dimension/mod.rs @@ -232,11 +232,12 @@ where /// units of `A` and in units of bytes between the least address and greatest /// address accessible by moving along all axes does not exceed `isize::MAX`. /// -/// Warning: This function is sufficient to check the invariants of ArrayBase only -/// if the pointer to the first element of the array is chosen such that the element -/// with the smallest memory address is at the start of data. (In other words, the -/// pointer to the first element of the array must be computed using offset_from_ptr_to_memory -/// so that negative strides are correctly handled.) +/// Warning: This function is sufficient to check the invariants of ArrayBase +/// only if the pointer to the first element of the array is chosen such that +/// the element with the smallest memory address is at the start of allocation. +/// (In other words, the pointer to the first element of the array must be +/// computed using `offset_from_low_addr_ptr_to_logical_ptr` so that negative +/// strides are correctly handled.) pub(crate) fn can_index_slice( data: &[A], dim: &D, @@ -404,17 +405,18 @@ fn to_abs_slice(axis_len: usize, slice: Slice) -> (usize, usize, isize) { (start, end, step) } -/// This function computes the offset from the logically first element to the first element in -/// memory of the array. The result is always <= 0. -pub fn offset_from_ptr_to_memory(dim: &D, strides: &D) -> isize { +/// This function computes the offset from the lowest address element to the +/// logically first element. The result is always >= 0. +pub fn offset_from_low_addr_ptr_to_logical_ptr(dim: &D, strides: &D) -> usize { let offset = izip!(dim.slice(), strides.slice()).fold(0, |_offset, (d, s)| { if (*s as isize) < 0 { - _offset + *s as isize * (*d as isize - 1) + _offset - *s as isize * (*d as isize - 1) } else { _offset } }); - offset + debug_assert!(offset >= 0); + offset as usize } /// Modify dimension, stride and return data pointer offset diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 3336ec336..40a54d1da 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -20,7 +20,7 @@ use alloc::vec; use alloc::vec::Vec; use crate::dimension; -use crate::dimension::offset_from_ptr_to_memory; +use crate::dimension::offset_from_low_addr_ptr_to_logical_ptr; use crate::error::{self, ShapeError}; use crate::extension::nonnull::nonnull_from_vec_data; use crate::imp_prelude::*; @@ -492,7 +492,7 @@ where // debug check for issues that indicates wrong use of this constructor debug_assert!(dimension::can_index_slice(&v, &dim, &strides).is_ok()); - let ptr = nonnull_from_vec_data(&mut v).offset(-offset_from_ptr_to_memory(&dim, &strides)); + let ptr = nonnull_from_vec_data(&mut v).add(offset_from_low_addr_ptr_to_logical_ptr(&dim, &strides)); ArrayBase::from_data_ptr(DataOwned::new(v), ptr).with_strides_dim(strides, dim) } diff --git a/src/impl_methods.rs b/src/impl_methods.rs index 9ef4277de..feada44ce 100644 --- a/src/impl_methods.rs +++ b/src/impl_methods.rs @@ -20,7 +20,7 @@ use crate::dimension; use crate::dimension::IntoDimension; use crate::dimension::{ abs_index, axes_of, do_slice, merge_axes, move_min_stride_axis_to_last, - offset_from_ptr_to_memory, size_of_shape_checked, stride_offset, Axes, + offset_from_low_addr_ptr_to_logical_ptr, size_of_shape_checked, stride_offset, Axes, }; use crate::dimension::broadcast::co_broadcast; use crate::error::{self, ErrorKind, ShapeError, from_kind}; @@ -1536,10 +1536,10 @@ where S: Data, { if self.is_contiguous() { - let offset = offset_from_ptr_to_memory(&self.dim, &self.strides); + let offset = offset_from_low_addr_ptr_to_logical_ptr(&self.dim, &self.strides); unsafe { Some(slice::from_raw_parts( - self.ptr.offset(offset).as_ptr(), + self.ptr.sub(offset).as_ptr(), self.len(), )) } @@ -1565,10 +1565,10 @@ where { if self.is_contiguous() { self.ensure_unique(); - let offset = offset_from_ptr_to_memory(&self.dim, &self.strides); + let offset = offset_from_low_addr_ptr_to_logical_ptr(&self.dim, &self.strides); unsafe { Ok(slice::from_raw_parts_mut( - self.ptr.offset(offset).as_ptr(), + self.ptr.sub(offset).as_ptr(), self.len(), )) } diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 441ebf4fb..e7bf761a0 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -101,13 +101,16 @@ where if self.is_empty() { return None; } - let offset = if std::mem::size_of::() == 0 { - -dimension::offset_from_ptr_to_memory(&self.dim, &self.strides) + if std::mem::size_of::() == 0 { + Some(dimension::offset_from_low_addr_ptr_to_logical_ptr( + &self.dim, + &self.strides, + )) } else { - unsafe { self.as_ptr().offset_from(self.data.as_ptr()) } - }; - debug_assert!(offset >= 0); - Some(offset as usize) + let offset = unsafe { self.as_ptr().offset_from(self.data.as_ptr()) }; + debug_assert!(offset >= 0); + Some(offset as usize) + } } /// Return a vector of the elements in the array, in the way they are diff --git a/src/impl_views/constructors.rs b/src/impl_views/constructors.rs index fd9457fa1..98cb81fcc 100644 --- a/src/impl_views/constructors.rs +++ b/src/impl_views/constructors.rs @@ -13,7 +13,7 @@ use crate::error::ShapeError; use crate::extension::nonnull::nonnull_debug_checked_from_ptr; use crate::imp_prelude::*; use crate::{is_aligned, StrideShape}; -use crate::dimension::offset_from_ptr_to_memory; +use crate::dimension::offset_from_low_addr_ptr_to_logical_ptr; /// Methods for read-only array views. impl<'a, A, D> ArrayView<'a, A, D> @@ -57,7 +57,7 @@ where let dim = shape.dim; dimension::can_index_slice_with_strides(xs, &dim, &shape.strides)?; let strides = shape.strides.strides_for_dim(&dim); - unsafe { Ok(Self::new_(xs.as_ptr().offset(-offset_from_ptr_to_memory(&dim, &strides)), dim, strides)) } + unsafe { Ok(Self::new_(xs.as_ptr().add(offset_from_low_addr_ptr_to_logical_ptr(&dim, &strides)), dim, strides)) } } /// Create an `ArrayView` from shape information and a raw pointer to @@ -154,7 +154,7 @@ where let dim = shape.dim; dimension::can_index_slice_with_strides(xs, &dim, &shape.strides)?; let strides = shape.strides.strides_for_dim(&dim); - unsafe { Ok(Self::new_(xs.as_mut_ptr().offset(-offset_from_ptr_to_memory(&dim, &strides)), dim, strides)) } + unsafe { Ok(Self::new_(xs.as_mut_ptr().add(offset_from_low_addr_ptr_to_logical_ptr(&dim, &strides)), dim, strides)) } } /// Create an `ArrayViewMut` from shape information and a From c33e03db945027ef735142082fd040e967fe3984 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 9 May 2021 15:16:40 -0400 Subject: [PATCH 4/6] Rename offset_to_first_elem to offset_from_alloc_to_logical_ptr --- src/impl_owned_array.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index e7bf761a0..b3889c2be 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -75,7 +75,7 @@ where /// arr.slice_axis_inplace(Axis(0), (1..).into()); /// arr[[0, 0]] = 5.; /// - /// let offset = arr.offset_to_first_elem().unwrap(); + /// let offset = arr.offset_from_alloc_to_logical_ptr().unwrap(); /// assert_eq!(offset, 4); /// /// let v = arr.into_raw_vec(); @@ -94,10 +94,10 @@ where /// /// let mut arr: Array2<()> = Array2::from_elem([3, 4], ()); /// arr.slice_axis_inplace(Axis(0), (1..).into()); - /// let offset = arr.offset_to_first_elem().unwrap(); + /// let offset = arr.offset_from_alloc_to_logical_ptr().unwrap(); /// assert_eq!(offset, 0); /// ``` - pub fn offset_to_first_elem(&self) -> Option { + pub fn offset_from_alloc_to_logical_ptr(&self) -> Option { if self.is_empty() { return None; } @@ -543,8 +543,8 @@ impl Array unsafe { // grow backing storage and update head ptr - let data_to_array_offset = self.offset_to_first_elem().unwrap_or(0); - self.ptr = self.data.reserve(len_to_append).add(data_to_array_offset); + let offset_from_alloc_to_logical = self.offset_from_alloc_to_logical_ptr().unwrap_or(0); + self.ptr = self.data.reserve(len_to_append).add(offset_from_alloc_to_logical); // clone elements from view to the array now // From e496e6318983ef1526dee79eb35b1ca8316f1769 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 9 May 2021 15:44:33 -0400 Subject: [PATCH 5/6] Add offset to return value of into_raw_vec --- examples/sort-axis.rs | 2 +- src/impl_owned_array.rs | 103 ++++++++++++++++++++++++--------------- tests/array-construct.rs | 4 +- tests/array.rs | 6 +-- 4 files changed, 70 insertions(+), 45 deletions(-) diff --git a/examples/sort-axis.rs b/examples/sort-axis.rs index ee5ff3465..f8bfadc13 100644 --- a/examples/sort-axis.rs +++ b/examples/sort-axis.rs @@ -155,7 +155,7 @@ where }); debug_assert_eq!(result.len(), moved_elements); // forget the old elements but not the allocation - let mut old_storage = self.into_raw_vec(); + let mut old_storage = self.into_raw_vec().0; old_storage.set_len(0); // transfer ownership of the elements into the result diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index b3889c2be..13cfc984d 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -61,42 +61,8 @@ impl Array where D: Dimension, { - /// Returns the offset (in units of `A`) from the start of the raw data to - /// the first element, or `None` if the array is empty. - /// - /// In other words, after converting the array to `Vec` with - /// [`.into_raw_vec()`](Self::into_raw_vec), this is the offset from the - /// start of the `Vec` to the first element of the array. - /// - /// ``` - /// use ndarray::{Array2, Axis}; - /// - /// let mut arr: Array2 = Array2::zeros([3, 4]); - /// arr.slice_axis_inplace(Axis(0), (1..).into()); - /// arr[[0, 0]] = 5.; - /// - /// let offset = arr.offset_from_alloc_to_logical_ptr().unwrap(); - /// assert_eq!(offset, 4); - /// - /// let v = arr.into_raw_vec(); - /// assert_eq!(v[offset], 5.); - /// ``` - /// - /// In the case of zero-sized elements, the offset is somewhat meaningless. - /// For convenience, an offset will be returned such that all indices - /// computed using the offset, shape, and strides will be in-bounds for the - /// `Vec` returned by [`.into_raw_vec()`](Self::into_raw_vec). Note that - /// this offset won't necessarily be the same as the offset for an array of - /// nonzero-sized elements sliced in the same way. - /// - /// ``` - /// use ndarray::{Array2, Axis}; - /// - /// let mut arr: Array2<()> = Array2::from_elem([3, 4], ()); - /// arr.slice_axis_inplace(Axis(0), (1..).into()); - /// let offset = arr.offset_from_alloc_to_logical_ptr().unwrap(); - /// assert_eq!(offset, 0); - /// ``` + /// Returns the offset (in units of `A`) from the start of the allocation + /// to the first element, or `None` if the array is empty. pub fn offset_from_alloc_to_logical_ptr(&self) -> Option { if self.is_empty() { return None; @@ -114,12 +80,71 @@ where } /// Return a vector of the elements in the array, in the way they are - /// stored internally. + /// stored internally, and the index in the vector corresponding to the + /// logically first element of the array (or `None` if the array is empty). /// /// If the array is in standard memory layout, the logical element order /// of the array (`.iter()` order) and of the returned vector will be the same. - pub fn into_raw_vec(self) -> Vec { - self.data.into_vec() + /// + /// ``` + /// use ndarray::{array, Array2, Axis}; + /// + /// let mut arr: Array2 = array![[1., 2.], [3., 4.], [5., 6.]]; + /// arr.slice_axis_inplace(Axis(0), (1..).into()); + /// assert_eq!(arr[[0, 0]], 3.); + /// let copy = arr.clone(); + /// + /// let shape = arr.shape().to_owned(); + /// let strides = arr.strides().to_owned(); + /// let (v, offset) = arr.into_raw_vec(); + /// + /// assert_eq!(v, &[1., 2., 3., 4., 5., 6.]); + /// assert_eq!(offset, Some(2)); + /// assert_eq!(v[offset.unwrap()], 3.); + /// for row in 0..shape[0] { + /// for col in 0..shape[1] { + /// let index = ( + /// offset.unwrap() as isize + /// + row as isize * strides[0] + /// + col as isize * strides[1] + /// ) as usize; + /// assert_eq!(v[index], copy[[row, col]]); + /// } + /// } + /// ``` + /// + /// In the case of zero-sized elements, the offset to the logically first + /// element is somewhat meaningless. For convenience, an offset will be + /// returned such that all indices computed using the offset, shape, and + /// strides will be in-bounds for the `Vec`. Note that this offset won't + /// necessarily be the same as the offset for an array of nonzero-sized + /// elements sliced in the same way. + /// + /// ``` + /// use ndarray::{array, Array2, Axis}; + /// + /// let mut arr: Array2<()> = array![[(), ()], [(), ()], [(), ()]]; + /// arr.slice_axis_inplace(Axis(0), (1..).into()); + /// + /// let shape = arr.shape().to_owned(); + /// let strides = arr.strides().to_owned(); + /// let (v, offset) = arr.into_raw_vec(); + /// + /// assert_eq!(v, &[(), (), (), (), (), ()]); + /// for row in 0..shape[0] { + /// for col in 0..shape[1] { + /// let index = ( + /// offset.unwrap() as isize + /// + row as isize * strides[0] + /// + col as isize * strides[1] + /// ) as usize; + /// assert_eq!(v[index], ()); + /// } + /// } + /// ``` + pub fn into_raw_vec(self) -> (Vec, Option) { + let offset = self.offset_from_alloc_to_logical_ptr(); + (self.data.into_vec(), offset) } } diff --git a/tests/array-construct.rs b/tests/array-construct.rs index 2b72ab039..3b4886ac5 100644 --- a/tests/array-construct.rs +++ b/tests/array-construct.rs @@ -21,9 +21,9 @@ fn test_from_shape_fn() { #[test] fn test_dimension_zero() { let a: Array2 = Array2::from(vec![[], [], []]); - assert_eq!(vec![0.; 0], a.into_raw_vec()); + assert_eq!(vec![0.; 0], a.into_raw_vec().0); let a: Array3 = Array3::from(vec![[[]], [[]], [[]]]); - assert_eq!(vec![0.; 0], a.into_raw_vec()); + assert_eq!(vec![0.; 0], a.into_raw_vec().0); } #[test] diff --git a/tests/array.rs b/tests/array.rs index 976824dfe..f19008ee9 100644 --- a/tests/array.rs +++ b/tests/array.rs @@ -1005,7 +1005,7 @@ fn array0_into_scalar() { // With this kind of setup, the `Array`'s pointer is not the same as the // underlying `Vec`'s pointer. let a: Array0 = array![4, 5, 6, 7].index_axis_move(Axis(0), 2); - assert_ne!(a.as_ptr(), a.into_raw_vec().as_ptr()); + assert_ne!(a.as_ptr(), a.into_raw_vec().0.as_ptr()); // `.into_scalar()` should still work correctly. let a: Array0 = array![4, 5, 6, 7].index_axis_move(Axis(0), 2); assert_eq!(a.into_scalar(), 6); @@ -1020,7 +1020,7 @@ fn array_view0_into_scalar() { // With this kind of setup, the `Array`'s pointer is not the same as the // underlying `Vec`'s pointer. let a: Array0 = array![4, 5, 6, 7].index_axis_move(Axis(0), 2); - assert_ne!(a.as_ptr(), a.into_raw_vec().as_ptr()); + assert_ne!(a.as_ptr(), a.into_raw_vec().0.as_ptr()); // `.into_scalar()` should still work correctly. let a: Array0 = array![4, 5, 6, 7].index_axis_move(Axis(0), 2); assert_eq!(a.view().into_scalar(), &6); @@ -1035,7 +1035,7 @@ fn array_view_mut0_into_scalar() { // With this kind of setup, the `Array`'s pointer is not the same as the // underlying `Vec`'s pointer. let a: Array0 = array![4, 5, 6, 7].index_axis_move(Axis(0), 2); - assert_ne!(a.as_ptr(), a.into_raw_vec().as_ptr()); + assert_ne!(a.as_ptr(), a.into_raw_vec().0.as_ptr()); // `.into_scalar()` should still work correctly. let mut a: Array0 = array![4, 5, 6, 7].index_axis_move(Axis(0), 2); assert_eq!(a.view_mut().into_scalar(), &6); From b8144e1a71e7f14bc189cda28d61d6839fc23a0f Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 9 May 2021 15:45:02 -0400 Subject: [PATCH 6/6] Make offset_from_alloc_to_logical_ptr private --- src/impl_owned_array.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/impl_owned_array.rs b/src/impl_owned_array.rs index 13cfc984d..4ac37e829 100644 --- a/src/impl_owned_array.rs +++ b/src/impl_owned_array.rs @@ -63,7 +63,7 @@ where { /// Returns the offset (in units of `A`) from the start of the allocation /// to the first element, or `None` if the array is empty. - pub fn offset_from_alloc_to_logical_ptr(&self) -> Option { + fn offset_from_alloc_to_logical_ptr(&self) -> Option { if self.is_empty() { return None; }