From 752ec38596cea9089098b4aa8f546b6e34bf15eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Fr=C3=B6jdh?= Date: Tue, 22 Sep 2026 09:25:42 +0200 Subject: [PATCH] added template for making views (#377) Cleaned up the helper functions by: - Added template for make_view (and new make_const_view) instead of the copy paste implementation - checks c_style and dimensions inside make_view - test binding to be able to probe the checks in make_view Call sites can be reviewed in a second PR since usage of const or strictness of conversion depends... --- python/src/bind_ClusterVector.hpp | 3 -- python/src/bind_testing.hpp | 65 ++++++++++++++++++++++++++ python/src/module.cpp | 2 + python/src/np_helper.hpp | 65 ++++++++++++++++++-------- python/tests/test_ClusterVector.py | 2 +- python/tests/test_make_view.py | 74 ++++++++++++++++++++++++++++++ 6 files changed, 189 insertions(+), 22 deletions(-) create mode 100644 python/src/bind_testing.hpp create mode 100644 python/tests/test_make_view.py diff --git a/python/src/bind_ClusterVector.hpp b/python/src/bind_ClusterVector.hpp index 095c3f8a..c6888a4a 100644 --- a/python/src/bind_ClusterVector.hpp +++ b/python/src/bind_ClusterVector.hpp @@ -48,9 +48,6 @@ void define_ClusterVector(py::module &m, const std::string &typestr) { "__call__", [](ClusterVector &self, py::array_t mask) { - if (mask.ndim() != 1) { - throw py::value_error("Mask must be one-dimensional"); - } return self(make_view_1d(mask)); }, py::arg("mask").noconvert(), R"doc( diff --git a/python/src/bind_testing.hpp b/python/src/bind_testing.hpp new file mode 100644 index 00000000..2a4e8760 --- /dev/null +++ b/python/src/bind_testing.hpp @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: MPL-2.0 +// Probes used by the python test-suite to exercise the helpers in +// np_helper.hpp. Internal, no API guarantees. +#pragma once + +#include "aare/NDArray.hpp" +#include "aare/NDView.hpp" +#include "np_helper.hpp" + +#include +#include +#include +#include +#include +#include + +namespace py = pybind11; +using namespace ::aare; + +// Arrays are taken with the default flags (no c_style) so that nothing is +// made contiguous by pybind11 before it reaches make_view +template +void define_make_view_probes(py::module &m, const std::string &suffix) { + + // Copy what C++ sees through the view back to python + m.def(("view_roundtrip_" + suffix).c_str(), + [](py::array_t arr) { + auto view = make_view(arr); + return return_image_data(new NDArray(view)); + }, + py::arg("arr").noconvert()); + + // Same as above but through a read only view + m.def(("const_view_roundtrip_" + suffix).c_str(), + [](py::array_t arr) { + auto view = make_const_view(arr); + static_assert( + std::is_same_v>); + auto out = new NDArray(view.shape()); + std::copy(view.begin(), view.end(), out->begin()); + return return_image_data(out); + }, + py::arg("arr").noconvert()); + + // Write through the view + m.def(("view_fill_" + suffix).c_str(), + [](py::array_t arr, T value) { + auto view = make_view(arr); + view = value; + }, + py::arg("arr").noconvert(), py::arg("value")); +} + +void define_testing_bindings(py::module &m) { + auto testing = m.def_submodule( + "testing", "Internal helpers for the test-suite, no API guarantees"); + + // noconvert + overloads gives dispatch on dtype + define_make_view_probes(testing, "1d"); + define_make_view_probes(testing, "2d"); + define_make_view_probes(testing, "3d"); + define_make_view_probes(testing, "1d"); + define_make_view_probes(testing, "2d"); + define_make_view_probes(testing, "3d"); +} diff --git a/python/src/module.cpp b/python/src/module.cpp index 8e39d064..b83deaca 100644 --- a/python/src/module.cpp +++ b/python/src/module.cpp @@ -22,6 +22,7 @@ #include "bind_PixelMap.hpp" #include "bind_RawFile.hpp" #include "bind_calibration.hpp" +#include "bind_testing.hpp" // TODO! migrate the other names #include "ctb_raw_file.hpp" @@ -88,6 +89,7 @@ PYBIND11_MODULE(_aare, m) { define_jungfrau_data_file_io_bindings(m); bind_calibration(m); + define_testing_bindings(m); DEFINE_CLUSTER_BINDINGS(int, 3, 3, uint16_t, i); DEFINE_CLUSTER_BINDINGS(double, 3, 3, uint16_t, d); diff --git a/python/src/np_helper.hpp b/python/src/np_helper.hpp index c0f5c3fd..d21aceff 100644 --- a/python/src/np_helper.hpp +++ b/python/src/np_helper.hpp @@ -41,31 +41,60 @@ template py::array return_vector(std::vector *vec) { free_when_done); // numpy array references this parent } -// todo rewrite generic -template -auto get_shape_3d(const py::array_t &arr) { - return aare::Shape<3>{arr.shape(0), arr.shape(1), arr.shape(2)}; +// Create a NDView of a numpy array. NDView assumes C-order strides so we +// reject anything that is not Ndim dimensional and C-contiguous instead of +// silently reading the data in the wrong order. +template +auto get_checked_shape(const py::array_t &arr) { + if (arr.ndim() != Ndim) { + throw py::value_error( + fmt::format("Expected {}D array, got {}D", Ndim, arr.ndim())); + } + if (!(arr.flags() & py::array::c_style)) { + throw py::value_error( + "Array is not C-contiguous. Use np.ascontiguousarray(arr)"); + } + aare::Shape shape{}; + for (ssize_t i = 0; i < Ndim; ++i) { + shape[i] = arr.shape(i); + } + return shape; +} + +template +auto make_view(py::array_t &arr) { + auto shape = get_checked_shape(arr); + return aare::NDView(arr.mutable_data(), shape); +} + +// Read only view, also works for numpy arrays that are not writeable +template +auto make_const_view(const py::array_t &arr) { + auto shape = get_checked_shape(arr); + return aare::NDView(arr.data(), shape); } template auto make_view_3d(py::array_t &arr) { - return aare::NDView(arr.mutable_data(), get_shape_3d(arr)); + return make_view<3>(arr); } - -template -auto get_shape_2d(const py::array_t &arr) { - return aare::Shape<2>{arr.shape(0), arr.shape(1)}; -} - -template -auto get_shape_1d(const py::array_t &arr) { - return aare::Shape<1>{arr.shape(0)}; -} - template auto make_view_2d(py::array_t &arr) { - return aare::NDView(arr.mutable_data(), get_shape_2d(arr)); + return make_view<2>(arr); } template auto make_view_1d(py::array_t &arr) { - return aare::NDView(arr.mutable_data(), get_shape_1d(arr)); + return make_view<1>(arr); +} + +template +auto make_const_view_3d(const py::array_t &arr) { + return make_const_view<3>(arr); +} +template +auto make_const_view_2d(const py::array_t &arr) { + return make_const_view<2>(arr); +} +template +auto make_const_view_1d(const py::array_t &arr) { + return make_const_view<1>(arr); } template struct fmt_format_trait; // forward declaration diff --git a/python/tests/test_ClusterVector.py b/python/tests/test_ClusterVector.py index 8b077dc0..dd75366e 100644 --- a/python/tests/test_ClusterVector.py +++ b/python/tests/test_ClusterVector.py @@ -157,5 +157,5 @@ def test_masking_requires_one_dimension(): mask = np.array([[True, False]], dtype=bool) - with pytest.raises(ValueError, match="one-dimensional"): + with pytest.raises(ValueError): cv(mask) diff --git a/python/tests/test_make_view.py b/python/tests/test_make_view.py new file mode 100644 index 00000000..2df947ca --- /dev/null +++ b/python/tests/test_make_view.py @@ -0,0 +1,74 @@ +# SPDX-License-Identifier: MPL-2.0 +"""Probe make_view in np_helper.hpp through the bindings in _aare.testing""" +import numpy as np +import pytest + +from aare._aare import testing + + + + +def test_make_view_roundtrip(): + """Send an array to C++ and check that we get the same array back""" + arr = np.arange(30, dtype=np.float64).reshape(5, 6) + out = testing.view_roundtrip_2d(arr) + np.testing.assert_array_equal(out, arr) + + #Sanity check to make sure we got a copy an not just the same thing back + arr[3,2] = 31655 + assert arr[3,2] != out[3,2] + +def test_make_view_const_roundtrip(): + """Const works if the array is not writeable""" + arr = np.arange(30, dtype=np.float64).reshape(5, 6) + arr.flags.writeable = False + out = testing.const_view_roundtrip_2d(arr) + np.testing.assert_array_equal(out, arr) + +def test_make_view_const_roundtrip_writable(): + """Const works also if the array is writeable""" + arr = np.arange(30, dtype=np.float64).reshape(5, 6) + out = testing.const_view_roundtrip_2d(arr) + np.testing.assert_array_equal(out, arr) + +def test_make_view_fill(): + arr = np.arange(30, dtype=np.float64).reshape(5, 6) + testing.view_fill_2d(arr, 7) + assert (arr == 7).all() + +def test_make_view_fill_rejects_nonwriteable(): + arr = np.arange(30, dtype=np.float64).reshape(5, 6) + arr.flags.writeable = False + with pytest.raises(ValueError, match="not writeable"): + testing.view_fill_2d(arr, 7) + +def test_make_view_2d_rejects_1d(): + arr = np.arange(30, dtype=np.uint16) + with pytest.raises(ValueError, match="Expected 2D array, got 1D"): + testing.view_roundtrip_2d(arr) + +def test_make_view_2d_rejects_3d(): + arr = np.arange(30, dtype=np.uint16).reshape(3, 5, 2) + with pytest.raises(ValueError, match="Expected 2D array, got 3D"): + testing.view_roundtrip_2d(arr) + +def test_make_view_rejects_noncontiguous(): + arr = np.arange(30, dtype=np.float64).reshape(5, 6) + arr = arr[::2, ::3] # non-contiguous + assert not arr.flags.c_contiguous + with pytest.raises(ValueError, match="not C-contiguous"): + testing.view_roundtrip_2d(arr) + +def test_make_view_rejects_noncontiguous_const(): + arr = np.arange(30, dtype=np.float64).reshape(5, 6) + arr = arr[::2, ::3] # non-contiguous + assert not arr.flags.c_contiguous + with pytest.raises(ValueError, match="not C-contiguous"): + testing.const_view_roundtrip_2d(arr) + +def test_make_view_rejects_fortran_order(): + arr = np.asfortranarray(np.arange(30, dtype=np.float64).reshape(5, 6)) + assert not arr.flags.c_contiguous + with pytest.raises(ValueError, match="not C-contiguous"): + testing.view_roundtrip_2d(arr) +