Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions src/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::cold;
use crate::convert::{ArrayExt, IntoPyArray, NpyIndex, ToNpyDims, ToPyArray};
use crate::dtype::{Element, PyArrayDescrMethods};
use crate::error::{
BorrowError, DimensionalityError, FromVecError, IgnoreError, NotContiguousError, TypeError,
AsSliceError, BorrowError, DimensionalityError, FromVecError, IgnoreError, TypeError,
DIMENSIONALITY_MISMATCH_ERR, MAX_DIMENSIONALITY_ERR,
};
use crate::npyffi::{self, npy_intp, NPY_ORDER, PY_ARRAY_API};
Expand Down Expand Up @@ -739,15 +739,22 @@ pub trait PyArrayMethods<'py, T, D>: PyUntypedArrayMethods<'py> + Sized {
/// or concurrently modified by Python or other native code.
///
/// Please consider the safe alternative [`PyReadonlyArray::as_slice`].
unsafe fn as_slice(&self) -> Result<&[T], NotContiguousError>
unsafe fn as_slice(&self) -> Result<&[T], AsSliceError>
where
T: Element,
D: Dimension,
{
if self.is_contiguous() {
Ok(slice::from_raw_parts(self.data(), self.len()))
let len = self.len();
if len == 0 {
// We can still produce a slice over zero objects regardless of whether
// the underlying pointer is aligned or not.
Ok(&[])
} else if !self.is_aligned() {
Err(AsSliceError::NotAligned)
} else if !self.is_contiguous() {
Err(AsSliceError::NotContiguous)
} else {
Err(NotContiguousError)
Ok(slice::from_raw_parts(self.data(), len))
}
}

Expand All @@ -761,15 +768,22 @@ pub trait PyArrayMethods<'py, T, D>: PyUntypedArrayMethods<'py> + Sized {
///
/// Please consider the safe alternative [`PyReadwriteArray::as_slice_mut`].
#[allow(clippy::mut_from_ref)]
unsafe fn as_slice_mut(&self) -> Result<&mut [T], NotContiguousError>
unsafe fn as_slice_mut(&self) -> Result<&mut [T], AsSliceError>
where
T: Element,
D: Dimension,
{
if self.is_contiguous() {
Ok(slice::from_raw_parts_mut(self.data(), self.len()))
let len = self.len();
if len == 0 {
// We can still produce a slice over zero objects regardless of whether
// the underlying pointer is aligned or not.
Ok(&mut [])
} else if !self.is_aligned() {
Err(AsSliceError::NotAligned)
} else if !self.is_contiguous() {
Err(AsSliceError::NotContiguous)
} else {
Err(NotContiguousError)
Ok(slice::from_raw_parts_mut(self.data(), len))
}
}

Expand Down Expand Up @@ -951,7 +965,7 @@ pub trait PyArrayMethods<'py, T, D>: PyUntypedArrayMethods<'py> + Sized {
/// })
/// # }
/// ```
fn to_vec(&self) -> Result<Vec<T>, NotContiguousError>
fn to_vec(&self) -> Result<Vec<T>, AsSliceError>
where
T: Element,
D: Dimension;
Expand Down Expand Up @@ -1503,7 +1517,7 @@ impl<'py, T, D> PyArrayMethods<'py, T, D> for Bound<'py, PyArray<T, D>> {
unsafe { self.cast_unchecked() }
}

fn to_vec(&self) -> Result<Vec<T>, NotContiguousError>
fn to_vec(&self) -> Result<Vec<T>, AsSliceError>
where
T: Element,
D: Dimension,
Expand Down
6 changes: 3 additions & 3 deletions src/borrow/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ use pyo3::{Borrowed, Bound, CastError, FromPyObject, PyAny, PyResult};
use crate::array::{PyArray, PyArrayMethods};
use crate::convert::NpyIndex;
use crate::dtype::Element;
use crate::error::{BorrowError, NotContiguousError};
use crate::error::{AsSliceError, BorrowError};
use crate::npyffi::flags;
use crate::untyped_array::PyUntypedArrayMethods;

Expand Down Expand Up @@ -268,7 +268,7 @@ where

/// Provide an immutable slice view of the interior of the NumPy array if it is contiguous.
#[inline(always)]
pub fn as_slice(&self) -> Result<&[T], NotContiguousError> {
pub fn as_slice(&self) -> Result<&[T], AsSliceError> {
// SAFETY: Global borrow flags ensure aliasing discipline.
unsafe { self.array.as_slice() }
}
Expand Down Expand Up @@ -511,7 +511,7 @@ where

/// Provide a mutable slice view of the interior of the NumPy array if it is contiguous.
#[inline(always)]
pub fn as_slice_mut(&mut self) -> Result<&mut [T], NotContiguousError> {
pub fn as_slice_mut(&mut self) -> Result<&mut [T], AsSliceError> {
// SAFETY: Global borrow flags ensure aliasing discipline.
unsafe { self.array.as_slice_mut() }
}
Expand Down
20 changes: 15 additions & 5 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,17 +141,27 @@ impl fmt::Display for FromVecError {

impl_pyerr!(FromVecError);

/// Represents that the given array is not contiguous.
/// Represents that the given array is not compatible with viewing as a Rust slice.
///
/// If an array fails for more than one reason, it is not guaranteed which variant is returned.
#[derive(Debug)]
pub struct NotContiguousError;
pub enum AsSliceError {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think adjusting the error type makes sense. Maybe we could consider adding #[non_exhaustive] now, so that we can extend this in the future in a non-breaking way if that happens to be necessary.

Alternatively we could also think about keeping this as an opaque error (just renamed and a new error message). Not sure if consumers would ever need to act differently depending on the condition, given that there is not much they can do about it. I tend to lean towards this, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of a situation where there'd be another (detectable) error in converting to a slice - the Rust rules are "aligned, contiguous and initialised", but initialised isn't detectable, and we already implicitly require Python space to behave safely for Rust around uninitialised data (no passing np.empty arrays, etc), so I'm not certain if non_exhaustive would be worth it.

When I made the enum I was roughly thinking that you can handle unaligned arrays differently to non-contiguous ones; you can cast an unaligned one to a pointer and do unaligned strided pointer reads through it to access the data. But that said, I guess the solution might more reasonably be to just make an infallible version of to_vec that does that misalignment and non-contiguity handling itself? In that model, we could revert the error back to an opaque "array is either misaligned or non-contiguous" message without needing to expose the complexity to users.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of a situation where there'd be another (detectable) error in converting to a slice - the Rust rules are "aligned, contiguous and initialised", but initialised isn't detectable, and we already implicitly require Python space to behave safely for Rust around uninitialised data (no passing np.empty arrays, etc), so I'm not certain if non_exhaustive would be worth it.

Makes sense.

But that said, I guess the solution might more reasonably be to just make an infallible version of to_vec that does that misalignment and non-contiguity handling itself?

That sounds like a nice idea. Maybe that could be to_cow, returning a borrowed slice if possible or an owned copy if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that sounds like a plan - let me pivot the error back to a single opaque struct, I'll leave the name as AsSliceError and make NonContiguousError a #[deprecated] type alias to it. I can open a new PR for PyArray::to_cow.

/// The array is not backed by an aligned pointer.
NotAligned,
/// The array is not contiguous in memory.
NotContiguous,
}

impl fmt::Display for NotContiguousError {
impl fmt::Display for AsSliceError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "The given array is not contiguous")
match self {
Self::NotAligned => write!(f, "The given array is not aligned"),
Self::NotContiguous => write!(f, "The given array is not contiguous"),
}
}
}

impl_pyerr!(NotContiguousError);
impl_pyerr!(AsSliceError);

/// Inidcates why borrowing an array failed.
#[derive(Debug)]
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ pub use crate::borrow::{
};
pub use crate::convert::{IntoPyArray, NpyIndex, ToNpyDims, ToPyArray};
pub use crate::dtype::{dtype, Complex32, Complex64, Element, PyArrayDescr, PyArrayDescrMethods};
pub use crate::error::{BorrowError, FromVecError, NotContiguousError};
pub use crate::error::{AsSliceError, BorrowError, FromVecError};
pub use crate::npyffi::{PY_ARRAY_API, PY_UFUNC_API};
pub use crate::strings::{PyFixedString, PyFixedUnicode};
pub use crate::sum_products::{dot, einsum, inner};
Expand Down
33 changes: 33 additions & 0 deletions src/untyped_array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,39 @@ pub trait PyUntypedArrayMethods<'py>: Sealed {
/// [PyArray_DTYPE]: https://numpy.org/doc/stable/reference/c-api/array.html#c.PyArray_DTYPE
fn dtype(&self) -> Bound<'py, PyArrayDescr>;

/// Returns `true` if the internal data of the array is aligned for the dtype.
///
/// Note that NumPy considers zero-length data to be aligned regardless of the base pointer,
/// which is a weaker condition than Rust's slice guarantees. [PyArrayMethods::as_slice] will
/// safely handle the case of a misaligned zero-length array.
///
/// # Example
///
/// ```
/// use numpy::{PyArray1, PyUntypedArrayMethods};
/// use pyo3::{types::{IntoPyDict, PyAnyMethods}, Python, ffi::c_str};
///
/// # fn main() -> pyo3::PyResult<()> {
/// Python::attach(|py| {
/// let array = PyArray1::<u16>::zeros(py, 8, false);
/// assert!(array.is_aligned());
///
/// let view = py
/// .eval(
/// c_str!("array.view('u1')[1:-1].view('u2')"),
/// None,
/// Some(&[("array", array)].into_py_dict(py)?),
/// )?
/// .cast_into::<PyArray1<u16>>()?;
/// assert!(!view.is_aligned());
/// # Ok(())
/// })
/// # }
/// ```
fn is_aligned(&self) -> bool {
unsafe { check_flags(&*self.as_array_ptr(), npyffi::NPY_ARRAY_ALIGNED) }
}

/// Returns `true` if the internal data of the array is contiguous,
/// indepedently of whether C-style/row-major or Fortran-style/column-major.
///
Expand Down
34 changes: 34 additions & 0 deletions tests/array.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ fn not_contiguous_array(py: Python<'_>) -> Bound<'_, PyArray1<i32>> {
.unwrap()
}

fn not_aligned_array(py: Python<'_>) -> Bound<'_, PyArray1<u16>> {
py.eval(
c_str!("np.zeros(8, dtype='u2').view('u1')[1:-1].view('u2')"),
None,
Some(&get_np_locals(py)),
)
.unwrap()
.cast_into()
.unwrap()
}

#[test]
fn new_c_order() {
Python::attach(|py| {
Expand All @@ -51,6 +62,7 @@ fn new_c_order() {
assert!(arr.is_contiguous());
assert!(arr.is_c_contiguous());
assert!(!arr.is_fortran_contiguous());
assert!(arr.is_aligned());
});
}

Expand All @@ -73,6 +85,7 @@ fn new_fortran_order() {
assert!(arr.is_contiguous());
assert!(!arr.is_c_contiguous());
assert!(arr.is_fortran_contiguous());
assert!(arr.is_aligned());
});
}

Expand Down Expand Up @@ -180,6 +193,27 @@ fn as_slice() {
let not_contiguous = not_contiguous_array(py);
let err = not_contiguous.readonly().as_slice().unwrap_err();
assert_eq!(err.to_string(), "The given array is not contiguous");

let not_aligned = not_aligned_array(py);
assert!(!not_aligned.is_aligned());
let err = not_aligned.readonly().as_slice().unwrap_err();
assert_eq!(err.to_string(), "The given array is not aligned");

let misaligned_empty: Bound<'_, PyArray1<u16>> = {
let arr = not_aligned_array(py);
py.eval(
c_str!("arr[0:0]"),
None,
Some(&[("arr", arr)].into_py_dict(py).unwrap()),
)
.unwrap()
.cast_into()
.unwrap()
};
assert_eq!(
misaligned_empty.readonly().as_slice().unwrap(),
&[] as &[u16]
);
});
}

Expand Down
Loading