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

@@ -48,9 +48,6 @@ void define_ClusterVector(py::module &m, const std::string &typestr) {
             "__call__",
             [](ClusterVector<ClusterType> &self,
                py::array_t<bool, py::array::c_style> 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(

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 <algorithm>
+#include <cstdint>
+#include <pybind11/numpy.h>
+#include <pybind11/pybind11.h>
+#include <pybind11/stl.h>
+#include <type_traits>
+
+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 <typename T, ssize_t Ndim>
+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<T> arr) {
+              auto view = make_view<Ndim>(arr);
+              return return_image_data(new NDArray<T, Ndim>(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<T> arr) {
+              auto view = make_const_view<Ndim>(arr);
+              static_assert(
+                  std::is_same_v<decltype(view), NDView<const T, Ndim>>);
+              auto out = new NDArray<T, Ndim>(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<T> arr, T value) {
+              auto view = make_view<Ndim>(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<double, 1>(testing, "1d");
+    define_make_view_probes<double, 2>(testing, "2d");
+    define_make_view_probes<double, 3>(testing, "3d");
+    define_make_view_probes<uint16_t, 1>(testing, "1d");
+    define_make_view_probes<uint16_t, 2>(testing, "2d");
+    define_make_view_probes<uint16_t, 3>(testing, "3d");
+}

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);

python/src/np_helper.hpp

@@ -41,31 +41,60 @@ template <typename T> py::array return_vector(std::vector<T> *vec) {
                           free_when_done); // numpy array references this parent
 }
 
-// todo rewrite generic
-template <class T, int Flags>
-auto get_shape_3d(const py::array_t<T, Flags> &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 <ssize_t Ndim, class T, int Flags>
+auto get_checked_shape(const py::array_t<T, Flags> &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<Ndim> shape{};
+    for (ssize_t i = 0; i < Ndim; ++i) {
+        shape[i] = arr.shape(i);
+    }
+    return shape;
+}
+
+template <ssize_t Ndim, class T, int Flags>
+auto make_view(py::array_t<T, Flags> &arr) {
+    auto shape = get_checked_shape<Ndim>(arr);
+    return aare::NDView<T, Ndim>(arr.mutable_data(), shape);
+}
+
+// Read only view, also works for numpy arrays that are not writeable
+template <ssize_t Ndim, class T, int Flags>
+auto make_const_view(const py::array_t<T, Flags> &arr) {
+    auto shape = get_checked_shape<Ndim>(arr);
+    return aare::NDView<const T, Ndim>(arr.data(), shape);
 }
 
 template <class T, int Flags> auto make_view_3d(py::array_t<T, Flags> &arr) {
-    return aare::NDView<T, 3>(arr.mutable_data(), get_shape_3d<T, Flags>(arr));
+    return make_view<3>(arr);
 }
-
-template <class T, int Flags>
-auto get_shape_2d(const py::array_t<T, Flags> &arr) {
-    return aare::Shape<2>{arr.shape(0), arr.shape(1)};
-}
-
-template <class T, int Flags>
-auto get_shape_1d(const py::array_t<T, Flags> &arr) {
-    return aare::Shape<1>{arr.shape(0)};
-}
-
 template <class T, int Flags> auto make_view_2d(py::array_t<T, Flags> &arr) {
-    return aare::NDView<T, 2>(arr.mutable_data(), get_shape_2d<T, Flags>(arr));
+    return make_view<2>(arr);
 }
 template <class T, int Flags> auto make_view_1d(py::array_t<T, Flags> &arr) {
-    return aare::NDView<T, 1>(arr.mutable_data(), get_shape_1d<T, Flags>(arr));
+    return make_view<1>(arr);
+}
+
+template <class T, int Flags>
+auto make_const_view_3d(const py::array_t<T, Flags> &arr) {
+    return make_const_view<3>(arr);
+}
+template <class T, int Flags>
+auto make_const_view_2d(const py::array_t<T, Flags> &arr) {
+    return make_const_view<2>(arr);
+}
+template <class T, int Flags>
+auto make_const_view_1d(const py::array_t<T, Flags> &arr) {
+    return make_const_view<1>(arr);
 }
 
 template <typename ClusterType> struct fmt_format_trait; // forward declaration

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)

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)
+