From 5168c135aec2668701b483789a32791947f9ac45 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 17 May 2023 09:14:15 -0700 Subject: [PATCH 01/24] Add `npy_format_descriptor` to enable `py::array_t` to/from-python conversions. --- include/pybind11/numpy.h | 26 +++++++++++++---- tests/test_numpy_array.cpp | 26 +++++++++++++++++ tests/test_numpy_array.py | 58 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 6 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 854d6e87fa..1a31fc292a 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -586,6 +586,16 @@ class dtype : public object { return detail::npy_format_descriptor::type>::dtype(); } + /// Return dtype for the given typenum (one of the NPY_TYPES). + /// https://numpy.org/devdocs/reference/c-api/array.html#c.PyArray_DescrFromType + static dtype from_typenum(int typenum) { + auto *ptr = detail::npy_api::get().PyArray_DescrFromType_(typenum); + if (!ptr) { + throw error_already_set(); + } + return reinterpret_steal(ptr); + } + /// Size of the data type in bytes. ssize_t itemsize() const { return detail::array_descriptor_proxy(m_ptr)->elsize; } @@ -1283,12 +1293,16 @@ struct npy_format_descriptor< public: static constexpr int value = values[detail::is_fmt_numeric::index]; - static pybind11::dtype dtype() { - if (auto *ptr = npy_api::get().PyArray_DescrFromType_(value)) { - return reinterpret_steal(ptr); - } - pybind11_fail("Unsupported buffer format!"); - } + static pybind11::dtype dtype() { return pybind11::dtype::from_typenum(value); } +}; + +template +struct npy_format_descriptor::value>> { + static constexpr auto name = const_name("object"); + + static constexpr int value = npy_api::NPY_OBJECT_; + + static pybind11::dtype dtype() { return pybind11::dtype::from_typenum(value); } }; #define PYBIND11_DECL_CHAR_FMT \ diff --git a/tests/test_numpy_array.cpp b/tests/test_numpy_array.cpp index b118e2c6cc..ef907dad6b 100644 --- a/tests/test_numpy_array.cpp +++ b/tests/test_numpy_array.cpp @@ -523,4 +523,30 @@ TEST_SUBMODULE(numpy_array, sm) { sm.def("test_fmt_desc_const_double", [](const py::array_t &) {}); sm.def("round_trip_float", [](double d) { return d; }); + + sm.def("pass_array_pyobject_ptr_return_sum_str_values", + [](const py::array_t &objs) { + std::string sum_str_values; + for (auto &obj : objs) { + sum_str_values += py::str(obj.attr("value")); + } + return sum_str_values; + }); + + sm.def("pass_array_pyobject_ptr_return_as_list", + [](const py::array_t &objs) -> py::list { return objs; }); + + sm.def("return_array_pyobject_ptr_cpp_loop", [](const py::list &objs) { + py::size_t arr_size = py::len(objs); + py::array_t arr_from_list(static_cast(arr_size)); + PyObject **data = arr_from_list.mutable_data(); + for (py::size_t i = 0; i < arr_size; i++) { + assert(data[i] == nullptr); + data[i] = py::cast(objs[i].attr("value")); + } + return arr_from_list; + }); + + sm.def("return_array_pyobject_ptr_from_list", + [](const py::list &objs) -> py::array_t { return objs; }); } diff --git a/tests/test_numpy_array.py b/tests/test_numpy_array.py index 070813d3a2..f9c4a10fcd 100644 --- a/tests/test_numpy_array.py +++ b/tests/test_numpy_array.py @@ -595,3 +595,61 @@ def test_round_trip_float(): arr = np.zeros((), np.float64) arr[()] = 37.2 assert m.round_trip_float(arr) == 37.2 + + +# For use as a temporary user-defined object, to maximize sensitivity of the tests below. +class PyValueHolder: + def __init__(self, value): + self.value = value + + +def WrapWithPyValueHolder(*values): + return [PyValueHolder(v) for v in values] + + +def UnwrapPyValueHolder(vhs): + return [vh.value for vh in vhs] + + +def test_pass_array_pyobject_ptr_return_sum_str_values_ndarray(): + # Intentionally all temporaries, do not change. + assert ( + m.pass_array_pyobject_ptr_return_sum_str_values( + np.array(WrapWithPyValueHolder(-3, "four", 5.0), dtype=object) + ) + == "-3four5.0" + ) + + +def test_pass_array_pyobject_ptr_return_sum_str_values_list(): + # Intentionally all temporaries, do not change. + assert ( + m.pass_array_pyobject_ptr_return_sum_str_values( + WrapWithPyValueHolder(2, "three", -4.0) + ) + == "2three-4.0" + ) + + +def test_pass_array_pyobject_ptr_return_as_list(): + # Intentionally all temporaries, do not change. + assert UnwrapPyValueHolder( + m.pass_array_pyobject_ptr_return_as_list( + np.array(WrapWithPyValueHolder(-1, "two", 3.0), dtype=object) + ) + ) == [-1, "two", 3.0] + + +@pytest.mark.parametrize( + ("return_array_pyobject_ptr", "unwrap"), + [ + (m.return_array_pyobject_ptr_cpp_loop, list), + (m.return_array_pyobject_ptr_from_list, UnwrapPyValueHolder), + ], +) +def test_return_array_pyobject_ptr_cpp_loop(return_array_pyobject_ptr, unwrap): + # Intentionally all temporaries, do not change. + arr_from_list = return_array_pyobject_ptr(WrapWithPyValueHolder(6, "seven", -8.0)) + assert isinstance(arr_from_list, np.ndarray) + assert arr_from_list.dtype == np.dtype("O") + assert unwrap(arr_from_list) == [6, "seven", -8.0] From d53a79611644a478a68c6c5e540e85f044d7c74b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 17 May 2023 15:53:57 -0700 Subject: [PATCH 02/24] resolve clang-tidy warning --- tests/test_numpy_array.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_numpy_array.cpp b/tests/test_numpy_array.cpp index ef907dad6b..8c122a8658 100644 --- a/tests/test_numpy_array.cpp +++ b/tests/test_numpy_array.cpp @@ -527,7 +527,7 @@ TEST_SUBMODULE(numpy_array, sm) { sm.def("pass_array_pyobject_ptr_return_sum_str_values", [](const py::array_t &objs) { std::string sum_str_values; - for (auto &obj : objs) { + for (const auto &obj : objs) { sum_str_values += py::str(obj.attr("value")); } return sum_str_values; From 5bea2a8b86f37e8b867e126dcd8eb011fc5b5844 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 17 May 2023 16:41:09 -0700 Subject: [PATCH 03/24] Use existing constructor instead of adding a static method. Thanks @Skylion007 for pointing out. --- include/pybind11/numpy.h | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index 1a31fc292a..36077ec04d 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -564,6 +564,8 @@ class dtype : public object { m_ptr = from_args(args).release().ptr(); } + /// Return dtype for the given typenum (one of the NPY_TYPES). + /// https://numpy.org/devdocs/reference/c-api/array.html#c.PyArray_DescrFromType explicit dtype(int typenum) : object(detail::npy_api::get().PyArray_DescrFromType_(typenum), stolen_t{}) { if (m_ptr == nullptr) { @@ -586,16 +588,6 @@ class dtype : public object { return detail::npy_format_descriptor::type>::dtype(); } - /// Return dtype for the given typenum (one of the NPY_TYPES). - /// https://numpy.org/devdocs/reference/c-api/array.html#c.PyArray_DescrFromType - static dtype from_typenum(int typenum) { - auto *ptr = detail::npy_api::get().PyArray_DescrFromType_(typenum); - if (!ptr) { - throw error_already_set(); - } - return reinterpret_steal(ptr); - } - /// Size of the data type in bytes. ssize_t itemsize() const { return detail::array_descriptor_proxy(m_ptr)->elsize; } @@ -1293,7 +1285,7 @@ struct npy_format_descriptor< public: static constexpr int value = values[detail::is_fmt_numeric::index]; - static pybind11::dtype dtype() { return pybind11::dtype::from_typenum(value); } + static pybind11::dtype dtype() { return pybind11::dtype(/*typenum*/ value); } }; template @@ -1302,7 +1294,7 @@ struct npy_format_descriptor Date: Wed, 17 May 2023 16:47:06 -0700 Subject: [PATCH 04/24] Add `format_descriptor` Trivial addition, but still in search for a meaningful test. --- include/pybind11/detail/common.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 1ff0df09fe..c2e6f9ec89 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -1025,6 +1025,15 @@ PYBIND11_RUNTIME_EXCEPTION(reference_cast_error, PyExc_RuntimeError) /// Used in template struct format_descriptor {}; +template +struct format_descriptor< + T, + detail::enable_if_t::value>> { + static constexpr const char c = 'O'; + static constexpr const char value[2] = {c, '\0'}; + static std::string format() { return std::string(1, c); } +}; + PYBIND11_NAMESPACE_BEGIN(detail) // Returns the index of the given type in the type char array below, and in the list in numpy.h // The order here is: bool; 8 ints ((signed,unsigned)x(8,16,32,64)bits); float,double,long double; From 82ce80fae193291e91bdaff8eb8a6e01441e49a9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 17 May 2023 17:52:43 -0700 Subject: [PATCH 05/24] Add test_format_descriptor_format --- tests/test_buffers.cpp | 28 ++++++++++++++++++++++++++++ tests/test_buffers.py | 26 ++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index 6b6e8cba7f..24fa46232b 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -7,12 +7,40 @@ BSD-style license that can be found in the LICENSE file. */ +#include #include #include "constructor_stats.h" #include "pybind11_tests.h" TEST_SUBMODULE(buffers, m) { + +#define PYBIND11_LOCAL_DEF(...) \ + if (cpp_name == #__VA_ARGS__) \ + return py::format_descriptor<__VA_ARGS__>::format(); + + m.def("format_descriptor_format", [](const std::string &cpp_name) { + PYBIND11_LOCAL_DEF(PyObject *) + PYBIND11_LOCAL_DEF(bool) + PYBIND11_LOCAL_DEF(std::int8_t) + PYBIND11_LOCAL_DEF(std::uint8_t) + PYBIND11_LOCAL_DEF(std::int16_t) + PYBIND11_LOCAL_DEF(std::uint16_t) + PYBIND11_LOCAL_DEF(std::int32_t) + PYBIND11_LOCAL_DEF(std::uint32_t) + PYBIND11_LOCAL_DEF(std::int64_t) + PYBIND11_LOCAL_DEF(std::uint64_t) + PYBIND11_LOCAL_DEF(float) + PYBIND11_LOCAL_DEF(double) + PYBIND11_LOCAL_DEF(long double) + PYBIND11_LOCAL_DEF(std::complex) + PYBIND11_LOCAL_DEF(std::complex) + PYBIND11_LOCAL_DEF(std::complex) + return std::string("UNKNOWN"); + }); + +#undef PYBIND11_LOCAL_DEF + // test_from_python / test_to_python: class Matrix { public: diff --git a/tests/test_buffers.py b/tests/test_buffers.py index eb58c4675e..943699c0de 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -11,6 +11,32 @@ np = pytest.importorskip("numpy") +@pytest.mark.parametrize( + ("cpp_name", "expected_codes"), + [ + ("PyObject *", ["O"]), + ("bool", ["?"]), + ("std::int8_t", ["b"]), + ("std::uint8_t", ["B"]), + ("std::int16_t", ["h"]), + ("std::uint16_t", ["H"]), + ("std::int32_t", ["i"]), + ("std::uint32_t", ["I"]), + ("std::int64_t", ["q"]), + ("std::uint64_t", ["Q"]), + ("float", ["f"]), + ("double", ["d"]), + ("long double", ["g", "d"]), + ("std::complex", ["Zf"]), + ("std::complex", ["Zd"]), + ("std::complex", ["Zg", "Zd"]), + ("", ["UNKNOWN"]), + ], +) +def test_format_descriptor_format(cpp_name, expected_codes): + assert m.format_descriptor_format(cpp_name) in expected_codes + + def test_from_python(): with pytest.raises(RuntimeError) as excinfo: m.Matrix(np.array([1, 2, 3])) # trying to assign a 1D array From 20b9baf270c719fbfaa4964592c59e1b2b8d34e4 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 01:37:04 -0700 Subject: [PATCH 06/24] Ensure the Eigen `type_caster`s do not segfault when loading arrays with dtype=object --- include/pybind11/eigen/matrix.h | 15 +++++++++++++++ include/pybind11/eigen/tensor.h | 10 ++++++++++ tests/test_eigen_matrix.cpp | 6 ++++++ tests/test_eigen_matrix.py | 17 +++++++++++++++++ tests/test_eigen_tensor.inl | 4 ++++ tests/test_eigen_tensor.py | 17 +++++++++++++++++ 6 files changed, 69 insertions(+) diff --git a/include/pybind11/eigen/matrix.h b/include/pybind11/eigen/matrix.h index bd533bed37..56241f43f5 100644 --- a/include/pybind11/eigen/matrix.h +++ b/include/pybind11/eigen/matrix.h @@ -290,6 +290,11 @@ struct type_caster::value>> { using props = EigenProps; bool load(handle src, bool convert) { + // dtype=object is not supported. See #1152 & #2259 for related experiments. + if (is_same_ignoring_cvref::value) { + return false; + } + // If we're in no-convert mode, only load if given an array of the correct type if (!convert && !isinstance>(src)) { return false; @@ -480,6 +485,11 @@ struct type_caster< public: bool load(handle src, bool convert) { + // dtype=object is not supported. See #1152 & #2259 for related experiments. + if (is_same_ignoring_cvref::value) { + return false; + } + // First check whether what we have is already an array of the right type. If not, we // can't avoid a copy (because the copy is also going to do type conversion). bool need_copy = !isinstance(src); @@ -637,6 +647,11 @@ struct type_caster::value>> { static constexpr bool rowMajor = Type::IsRowMajor; bool load(handle src, bool) { + // dtype=object is not supported. See #1152 & #2259 for related experiments. + if (is_same_ignoring_cvref::value) { + return false; + } + if (!src) { return false; } diff --git a/include/pybind11/eigen/tensor.h b/include/pybind11/eigen/tensor.h index de7dcba89d..4bdda0da4c 100644 --- a/include/pybind11/eigen/tensor.h +++ b/include/pybind11/eigen/tensor.h @@ -169,6 +169,11 @@ struct type_caster::ValidType> { PYBIND11_TYPE_CASTER(Type, temp_name); bool load(handle src, bool convert) { + // dtype=object is not supported. See #1152 & #2259 for related experiments. + if (is_same_ignoring_cvref::value) { + return false; + } + if (!convert) { if (!isinstance(src)) { return false; @@ -363,6 +368,11 @@ struct type_caster, using Helper = eigen_tensor_helper>; bool load(handle src, bool /*convert*/) { + // dtype=object is not supported. See #1152 & #2259 for related experiments. + if (is_same_ignoring_cvref::value) { + return false; + } + // Note that we have a lot more checks here as we want to make sure to avoid copies if (!isinstance(src)) { return false; diff --git a/tests/test_eigen_matrix.cpp b/tests/test_eigen_matrix.cpp index 554cc4d7f8..ab5a658f48 100644 --- a/tests/test_eigen_matrix.cpp +++ b/tests/test_eigen_matrix.cpp @@ -425,4 +425,10 @@ TEST_SUBMODULE(eigen_matrix, m) { py::module_::import("numpy").attr("ones")(10); return v[0](5); }); + + m.def("pass_eigen_matrix_dtype_object", + [](const Eigen::Matrix &) {}); + m.def("pass_eigen_ref_matrix_dtype_object", + [](const Eigen::Ref> &) {}); + m.def("pass_eigen_sparse_matrix_dtype_object", [](const Eigen::SparseMatrix &) {}); } diff --git a/tests/test_eigen_matrix.py b/tests/test_eigen_matrix.py index b2e76740b1..eccb9f8a25 100644 --- a/tests/test_eigen_matrix.py +++ b/tests/test_eigen_matrix.py @@ -805,3 +805,20 @@ def test_custom_operator_new(): o = m.CustomOperatorNew() np.testing.assert_allclose(o.a, 0.0) np.testing.assert_allclose(o.b.diagonal(), 1.0) + + +@pytest.mark.parametrize( + "pass_eigen_type_dtype_object", + [ + m.pass_eigen_matrix_dtype_object, + m.pass_eigen_ref_matrix_dtype_object, + m.pass_eigen_sparse_matrix_dtype_object, + ], +) +def test_pass_array_with_dtype_object(pass_eigen_type_dtype_object): + # Only the dtype matters (not shape etc.): dtype=object is (should be) the + # first check in the type_caster load() implementations. + obj = np.array([], dtype=object) + with pytest.raises(TypeError) as excinfo: + pass_eigen_type_dtype_object(obj) + assert "incompatible function arguments" in str(excinfo.value) diff --git a/tests/test_eigen_tensor.inl b/tests/test_eigen_tensor.inl index d864ce7379..27d7e86b2e 100644 --- a/tests/test_eigen_tensor.inl +++ b/tests/test_eigen_tensor.inl @@ -320,6 +320,10 @@ void init_tensor_module(pybind11::module &m) { "round_trip_rank_0_view", [](Eigen::TensorMap> &tensor) { return tensor; }, py::return_value_policy::reference); + + m.def("pass_eigen_tensor_dtype_object", [](const Eigen::Tensor &) {}); + m.def("pass_eigen_tensor_map_dtype_object", + [](Eigen::TensorMap> &) {}); } void test_module(py::module_ &m) { diff --git a/tests/test_eigen_tensor.py b/tests/test_eigen_tensor.py index 3e7ee6b7f2..05842acee7 100644 --- a/tests/test_eigen_tensor.py +++ b/tests/test_eigen_tensor.py @@ -286,3 +286,20 @@ def test_doc_string(m, doc): f"round_trip_const_view_tensor(arg0: numpy.ndarray[numpy.float64[?, ?, ?], {order_flag}])" " -> numpy.ndarray[numpy.float64[?, ?, ?]]" ) + + +@pytest.mark.parametrize("m", submodules) +@pytest.mark.parametrize( + "func_name", + [ + "pass_eigen_tensor_dtype_object", + "pass_eigen_tensor_map_dtype_object", + ], +) +def test_pass_array_with_dtype_object(m, func_name): + # Only the dtype matters (not shape etc.): dtype=object is (should be) the + # first check in the type_caster load() implementations. + obj = np.array([], dtype=object) + with pytest.raises(TypeError) as excinfo: + getattr(m, func_name)(obj) + assert "incompatible function arguments" in str(excinfo.value) From 0640eb335977d31862dd6bc95df90a402039cdaf Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 08:37:03 -0700 Subject: [PATCH 07/24] Use `static_assert()` `!std::is_pointer<>` to replace runtime guards. --- include/pybind11/detail/common.h | 5 +++++ include/pybind11/eigen/matrix.h | 27 ++++++++++++--------------- include/pybind11/eigen/tensor.h | 14 ++++---------- tests/test_eigen_matrix.cpp | 6 ------ tests/test_eigen_matrix.py | 17 ----------------- tests/test_eigen_tensor.inl | 4 ---- tests/test_eigen_tensor.py | 17 ----------------- 7 files changed, 21 insertions(+), 69 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index c2e6f9ec89..9e3ce60b4a 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -1034,6 +1034,11 @@ struct format_descriptor< static std::string format() { return std::string(1, c); } }; +// Common message for `static_assert()`s, which are useful to easily preempt much less obvious +// errors in code that does not support `format_descriptor`. +#define PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED \ + "Pointer types (in particular `PyObject *`) are not supported." + PYBIND11_NAMESPACE_BEGIN(detail) // Returns the index of the given type in the type char array below, and in the list in numpy.h // The order here is: bool; 8 ints ((signed,unsigned)x(8,16,32,64)bits); float,double,long double; diff --git a/include/pybind11/eigen/matrix.h b/include/pybind11/eigen/matrix.h index 56241f43f5..1f821476e1 100644 --- a/include/pybind11/eigen/matrix.h +++ b/include/pybind11/eigen/matrix.h @@ -287,14 +287,11 @@ handle eigen_encapsulate(Type *src) { template struct type_caster::value>> { using Scalar = typename Type::Scalar; + static_assert(!std::is_pointer::value, + PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using props = EigenProps; bool load(handle src, bool convert) { - // dtype=object is not supported. See #1152 & #2259 for related experiments. - if (is_same_ignoring_cvref::value) { - return false; - } - // If we're in no-convert mode, only load if given an array of the correct type if (!convert && !isinstance>(src)) { return false; @@ -410,6 +407,9 @@ struct type_caster::value>> { // Base class for casting reference/map/block/etc. objects back to python. template struct eigen_map_caster { + static_assert(!std::is_pointer::value, + PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); + private: using props = EigenProps; @@ -462,6 +462,8 @@ struct type_caster< using Type = Eigen::Ref; using props = EigenProps; using Scalar = typename props::Scalar; + static_assert(!std::is_pointer::value, + PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using MapType = Eigen::Map; using Array = array_t::value) { - return false; - } - // First check whether what we have is already an array of the right type. If not, we // can't avoid a copy (because the copy is also going to do type conversion). bool need_copy = !isinstance(src); @@ -614,6 +611,9 @@ struct type_caster< // regular Eigen::Matrix, then casting that. template struct type_caster::value>> { + static_assert(!std::is_pointer::value, + PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); + protected: using Matrix = Eigen::Matrix; @@ -642,16 +642,13 @@ struct type_caster::value>> { template struct type_caster::value>> { using Scalar = typename Type::Scalar; + static_assert(!std::is_pointer::value, + PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using StorageIndex = remove_reference_t().outerIndexPtr())>; using Index = typename Type::Index; static constexpr bool rowMajor = Type::IsRowMajor; bool load(handle src, bool) { - // dtype=object is not supported. See #1152 & #2259 for related experiments. - if (is_same_ignoring_cvref::value) { - return false; - } - if (!src) { return false; } diff --git a/include/pybind11/eigen/tensor.h b/include/pybind11/eigen/tensor.h index 4bdda0da4c..d067003a78 100644 --- a/include/pybind11/eigen/tensor.h +++ b/include/pybind11/eigen/tensor.h @@ -164,16 +164,13 @@ PYBIND11_WARNING_POP template struct type_caster::ValidType> { + static_assert(!std::is_pointer::value, + PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using Helper = eigen_tensor_helper; static constexpr auto temp_name = get_tensor_descriptor::value; PYBIND11_TYPE_CASTER(Type, temp_name); bool load(handle src, bool convert) { - // dtype=object is not supported. See #1152 & #2259 for related experiments. - if (is_same_ignoring_cvref::value) { - return false; - } - if (!convert) { if (!isinstance(src)) { return false; @@ -364,15 +361,12 @@ struct get_storage_pointer_type struct type_caster, typename eigen_tensor_helper>::ValidType> { + static_assert(!std::is_pointer::value, + PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using MapType = Eigen::TensorMap; using Helper = eigen_tensor_helper>; bool load(handle src, bool /*convert*/) { - // dtype=object is not supported. See #1152 & #2259 for related experiments. - if (is_same_ignoring_cvref::value) { - return false; - } - // Note that we have a lot more checks here as we want to make sure to avoid copies if (!isinstance(src)) { return false; diff --git a/tests/test_eigen_matrix.cpp b/tests/test_eigen_matrix.cpp index ab5a658f48..554cc4d7f8 100644 --- a/tests/test_eigen_matrix.cpp +++ b/tests/test_eigen_matrix.cpp @@ -425,10 +425,4 @@ TEST_SUBMODULE(eigen_matrix, m) { py::module_::import("numpy").attr("ones")(10); return v[0](5); }); - - m.def("pass_eigen_matrix_dtype_object", - [](const Eigen::Matrix &) {}); - m.def("pass_eigen_ref_matrix_dtype_object", - [](const Eigen::Ref> &) {}); - m.def("pass_eigen_sparse_matrix_dtype_object", [](const Eigen::SparseMatrix &) {}); } diff --git a/tests/test_eigen_matrix.py b/tests/test_eigen_matrix.py index eccb9f8a25..b2e76740b1 100644 --- a/tests/test_eigen_matrix.py +++ b/tests/test_eigen_matrix.py @@ -805,20 +805,3 @@ def test_custom_operator_new(): o = m.CustomOperatorNew() np.testing.assert_allclose(o.a, 0.0) np.testing.assert_allclose(o.b.diagonal(), 1.0) - - -@pytest.mark.parametrize( - "pass_eigen_type_dtype_object", - [ - m.pass_eigen_matrix_dtype_object, - m.pass_eigen_ref_matrix_dtype_object, - m.pass_eigen_sparse_matrix_dtype_object, - ], -) -def test_pass_array_with_dtype_object(pass_eigen_type_dtype_object): - # Only the dtype matters (not shape etc.): dtype=object is (should be) the - # first check in the type_caster load() implementations. - obj = np.array([], dtype=object) - with pytest.raises(TypeError) as excinfo: - pass_eigen_type_dtype_object(obj) - assert "incompatible function arguments" in str(excinfo.value) diff --git a/tests/test_eigen_tensor.inl b/tests/test_eigen_tensor.inl index 27d7e86b2e..d864ce7379 100644 --- a/tests/test_eigen_tensor.inl +++ b/tests/test_eigen_tensor.inl @@ -320,10 +320,6 @@ void init_tensor_module(pybind11::module &m) { "round_trip_rank_0_view", [](Eigen::TensorMap> &tensor) { return tensor; }, py::return_value_policy::reference); - - m.def("pass_eigen_tensor_dtype_object", [](const Eigen::Tensor &) {}); - m.def("pass_eigen_tensor_map_dtype_object", - [](Eigen::TensorMap> &) {}); } void test_module(py::module_ &m) { diff --git a/tests/test_eigen_tensor.py b/tests/test_eigen_tensor.py index 05842acee7..3e7ee6b7f2 100644 --- a/tests/test_eigen_tensor.py +++ b/tests/test_eigen_tensor.py @@ -286,20 +286,3 @@ def test_doc_string(m, doc): f"round_trip_const_view_tensor(arg0: numpy.ndarray[numpy.float64[?, ?, ?], {order_flag}])" " -> numpy.ndarray[numpy.float64[?, ?, ?]]" ) - - -@pytest.mark.parametrize("m", submodules) -@pytest.mark.parametrize( - "func_name", - [ - "pass_eigen_tensor_dtype_object", - "pass_eigen_tensor_map_dtype_object", - ], -) -def test_pass_array_with_dtype_object(m, func_name): - # Only the dtype matters (not shape etc.): dtype=object is (should be) the - # first check in the type_caster load() implementations. - obj = np.array([], dtype=object) - with pytest.raises(TypeError) as excinfo: - getattr(m, func_name)(obj) - assert "incompatible function arguments" in str(excinfo.value) From ddb625e69db2fa1f0c4966a8b0eb05f69ee85d97 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 10:08:35 -0700 Subject: [PATCH 08/24] Add comments to explain how to check for ref-count bugs. (NO code changes.) --- tests/test_numpy_array.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/test_numpy_array.py b/tests/test_numpy_array.py index f9c4a10fcd..12e7d17d15 100644 --- a/tests/test_numpy_array.py +++ b/tests/test_numpy_array.py @@ -597,7 +597,20 @@ def test_round_trip_float(): assert m.round_trip_float(arr) == 37.2 -# For use as a temporary user-defined object, to maximize sensitivity of the tests below. +# HINT: An easy and robust way (although only manual unfortunately) to check for +# ref-count leaks in the test_.*pyobject_ptr.* functions below is to +# * temporarily insert `while True:` (one-by-one), +# * run this test, and +# * run the Linux `top` command in another shell to visually monitor +# `RES` for a minute or two. +# If there is a leak, it is usually evident in seconds because the `RES` +# value increases without bounds. (Don't forget to Ctrl-C the test!) + + +# For use as a temporary user-defined object, to maximize sensitivity of the tests below: +# * Ref-count leaks will be immediately evident. +# * Sanitizers are much more likely to detect heap-use-after-free due to +# other ref-count bugs. class PyValueHolder: def __init__(self, value): self.value = value From 03dafde5cb2079bc582774b5d5970d8bbf5eeb4b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 13:44:22 -0700 Subject: [PATCH 09/24] Make the "Pointer types ... are not supported" message Eigen-specific, as suggested by @Lalaland. Move to new pybind11/eigen/common.h header. --- CMakeLists.txt | 1 + include/pybind11/detail/common.h | 5 ----- include/pybind11/eigen/common.h | 9 +++++++++ include/pybind11/eigen/matrix.h | 11 ++++++----- include/pybind11/eigen/tensor.h | 5 +++-- tests/extra_python_package/test_files.py | 1 + 6 files changed, 20 insertions(+), 12 deletions(-) create mode 100644 include/pybind11/eigen/common.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 25a7d14984..0ad74db2bb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -126,6 +126,7 @@ set(PYBIND11_HEADERS include/pybind11/complex.h include/pybind11/options.h include/pybind11/eigen.h + include/pybind11/eigen/common.h include/pybind11/eigen/matrix.h include/pybind11/eigen/tensor.h include/pybind11/embed.h diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 9e3ce60b4a..c2e6f9ec89 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -1034,11 +1034,6 @@ struct format_descriptor< static std::string format() { return std::string(1, c); } }; -// Common message for `static_assert()`s, which are useful to easily preempt much less obvious -// errors in code that does not support `format_descriptor`. -#define PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED \ - "Pointer types (in particular `PyObject *`) are not supported." - PYBIND11_NAMESPACE_BEGIN(detail) // Returns the index of the given type in the type char array below, and in the list in numpy.h // The order here is: bool; 8 ints ((signed,unsigned)x(8,16,32,64)bits); float,double,long double; diff --git a/include/pybind11/eigen/common.h b/include/pybind11/eigen/common.h new file mode 100644 index 0000000000..24f56d1584 --- /dev/null +++ b/include/pybind11/eigen/common.h @@ -0,0 +1,9 @@ +// Copyright (c) 2023 The pybind Community. + +#pragma once + +// Common message for `static_assert()`s, which are useful to easily +// preempt much less obvious errors. +#define PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED \ + "Pointer types (in particular `PyObject *`) are not supported as scalar types for Eigen " \ + "types." diff --git a/include/pybind11/eigen/matrix.h b/include/pybind11/eigen/matrix.h index 1f821476e1..8d4342f81b 100644 --- a/include/pybind11/eigen/matrix.h +++ b/include/pybind11/eigen/matrix.h @@ -10,6 +10,7 @@ #pragma once #include "../numpy.h" +#include "common.h" /* HINT: To suppress warnings originating from the Eigen headers, use -isystem. See also: @@ -288,7 +289,7 @@ template struct type_caster::value>> { using Scalar = typename Type::Scalar; static_assert(!std::is_pointer::value, - PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); + PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using props = EigenProps; bool load(handle src, bool convert) { @@ -408,7 +409,7 @@ struct type_caster::value>> { template struct eigen_map_caster { static_assert(!std::is_pointer::value, - PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); + PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); private: using props = EigenProps; @@ -463,7 +464,7 @@ struct type_caster< using props = EigenProps; using Scalar = typename props::Scalar; static_assert(!std::is_pointer::value, - PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); + PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using MapType = Eigen::Map; using Array = array_t struct type_caster::value>> { static_assert(!std::is_pointer::value, - PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); + PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); protected: using Matrix @@ -643,7 +644,7 @@ template struct type_caster::value>> { using Scalar = typename Type::Scalar; static_assert(!std::is_pointer::value, - PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); + PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using StorageIndex = remove_reference_t().outerIndexPtr())>; using Index = typename Type::Index; static constexpr bool rowMajor = Type::IsRowMajor; diff --git a/include/pybind11/eigen/tensor.h b/include/pybind11/eigen/tensor.h index d067003a78..25d12baca1 100644 --- a/include/pybind11/eigen/tensor.h +++ b/include/pybind11/eigen/tensor.h @@ -8,6 +8,7 @@ #pragma once #include "../numpy.h" +#include "common.h" #if defined(__GNUC__) && !defined(__clang__) && !defined(__INTEL_COMPILER) static_assert(__GNUC__ > 5, "Eigen Tensor support in pybind11 requires GCC > 5.0"); @@ -165,7 +166,7 @@ PYBIND11_WARNING_POP template struct type_caster::ValidType> { static_assert(!std::is_pointer::value, - PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); + PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using Helper = eigen_tensor_helper; static constexpr auto temp_name = get_tensor_descriptor::value; PYBIND11_TYPE_CASTER(Type, temp_name); @@ -362,7 +363,7 @@ template struct type_caster, typename eigen_tensor_helper>::ValidType> { static_assert(!std::is_pointer::value, - PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); + PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); using MapType = Eigen::TensorMap; using Helper = eigen_tensor_helper>; diff --git a/tests/extra_python_package/test_files.py b/tests/extra_python_package/test_files.py index e5982f962e..f3916ee1f9 100644 --- a/tests/extra_python_package/test_files.py +++ b/tests/extra_python_package/test_files.py @@ -57,6 +57,7 @@ } eigen_headers = { + "include/pybind11/eigen/common.h", "include/pybind11/eigen/matrix.h", "include/pybind11/eigen/tensor.h", } From 28492edc8358f3e06227ac99026161615d8e7613 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 15:22:07 -0700 Subject: [PATCH 10/24] Change "format_descriptor_format" implementation as suggested by @Lalaland. Additional tests meant to ensure consistency between py::format_descriptor<>, np.array, np.format_parser turn out to be useful only to highlight long-standing inconsistencies. --- tests/test_buffers.cpp | 48 ++++++++++++++++----------------- tests/test_buffers.py | 61 ++++++++++++++++++++++++++++-------------- 2 files changed, 65 insertions(+), 44 deletions(-) diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index 24fa46232b..c842f5fd52 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -14,33 +14,33 @@ #include "pybind11_tests.h" TEST_SUBMODULE(buffers, m) { - -#define PYBIND11_LOCAL_DEF(...) \ - if (cpp_name == #__VA_ARGS__) \ - return py::format_descriptor<__VA_ARGS__>::format(); - m.def("format_descriptor_format", [](const std::string &cpp_name) { - PYBIND11_LOCAL_DEF(PyObject *) - PYBIND11_LOCAL_DEF(bool) - PYBIND11_LOCAL_DEF(std::int8_t) - PYBIND11_LOCAL_DEF(std::uint8_t) - PYBIND11_LOCAL_DEF(std::int16_t) - PYBIND11_LOCAL_DEF(std::uint16_t) - PYBIND11_LOCAL_DEF(std::int32_t) - PYBIND11_LOCAL_DEF(std::uint32_t) - PYBIND11_LOCAL_DEF(std::int64_t) - PYBIND11_LOCAL_DEF(std::uint64_t) - PYBIND11_LOCAL_DEF(float) - PYBIND11_LOCAL_DEF(double) - PYBIND11_LOCAL_DEF(long double) - PYBIND11_LOCAL_DEF(std::complex) - PYBIND11_LOCAL_DEF(std::complex) - PYBIND11_LOCAL_DEF(std::complex) - return std::string("UNKNOWN"); + // https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables + static auto table = new std::map; + if (table->empty()) { +#define PYBIND11_ASSIGN_HELPER(...) \ + (*table)[#__VA_ARGS__] = py::format_descriptor<__VA_ARGS__>::format(); + PYBIND11_ASSIGN_HELPER(PyObject *) + PYBIND11_ASSIGN_HELPER(bool) + PYBIND11_ASSIGN_HELPER(std::int8_t) + PYBIND11_ASSIGN_HELPER(std::uint8_t) + PYBIND11_ASSIGN_HELPER(std::int16_t) + PYBIND11_ASSIGN_HELPER(std::uint16_t) + PYBIND11_ASSIGN_HELPER(std::int32_t) + PYBIND11_ASSIGN_HELPER(std::uint32_t) + PYBIND11_ASSIGN_HELPER(std::int64_t) + PYBIND11_ASSIGN_HELPER(std::uint64_t) + PYBIND11_ASSIGN_HELPER(float) + PYBIND11_ASSIGN_HELPER(double) + PYBIND11_ASSIGN_HELPER(long double) + PYBIND11_ASSIGN_HELPER(std::complex) + PYBIND11_ASSIGN_HELPER(std::complex) + PYBIND11_ASSIGN_HELPER(std::complex) +#undef PYBIND11_ASSIGN_HELPER + } + return (*table)[cpp_name]; }); -#undef PYBIND11_LOCAL_DEF - // test_from_python / test_to_python: class Matrix { public: diff --git a/tests/test_buffers.py b/tests/test_buffers.py index 943699c0de..c2795e1b7a 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -12,29 +12,50 @@ @pytest.mark.parametrize( - ("cpp_name", "expected_codes"), + ("cpp_name", "expected_fmts", "np_array_dtype"), [ - ("PyObject *", ["O"]), - ("bool", ["?"]), - ("std::int8_t", ["b"]), - ("std::uint8_t", ["B"]), - ("std::int16_t", ["h"]), - ("std::uint16_t", ["H"]), - ("std::int32_t", ["i"]), - ("std::uint32_t", ["I"]), - ("std::int64_t", ["q"]), - ("std::uint64_t", ["Q"]), - ("float", ["f"]), - ("double", ["d"]), - ("long double", ["g", "d"]), - ("std::complex", ["Zf"]), - ("std::complex", ["Zd"]), - ("std::complex", ["Zg", "Zd"]), - ("", ["UNKNOWN"]), + ("PyObject *", ["O"], object), + ("bool", ["?"], np.bool_), + ("std::int8_t", ["b"], np.int8), + ("std::uint8_t", ["B"], np.uint8), + ("std::int16_t", ["h"], np.int16), + ("std::uint16_t", ["H"], np.uint16), + ("std::int32_t", ["i"], np.int32), + ("std::uint32_t", ["I"], np.uint32), + ("std::int64_t", ["q"], np.int64), + ("std::uint64_t", ["Q"], np.uint64), + ("float", ["f"], np.float32), + ("double", ["d"], np.float64), + ("long double", ["g", "d"], np.float128), + ("std::complex", ["Zf"], np.complex64), + ("std::complex", ["Zd"], np.complex128), + ("std::complex", ["Zg", "Zd"], np.complex256), ], ) -def test_format_descriptor_format(cpp_name, expected_codes): - assert m.format_descriptor_format(cpp_name) in expected_codes +def test_format_descriptor_format(cpp_name, expected_fmts, np_array_dtype): + fmt = m.format_descriptor_format(cpp_name) + assert fmt in expected_fmts + + # Everything below just documents long-standing inconsistencies. + # See also: https://github.com/pybind/pybind11/issues/1908 + + # py::format_descriptor<> vs np.array: + na = np.array([], dtype=np_array_dtype) + bi = m.get_buffer_info(na) + if fmt == "q": + assert bi.format in ["q", "l"] + elif fmt == "Q": + assert bi.format in ["Q", "L"] + else: + assert bi.format == fmt + + # py::format_descriptor<> vs np.format_parser(): + fmtp = fmt[1:] if fmt.startswith("Z") else fmt + fp = np.format_parser(fmtp, [], []) + assert fp.dtype is not None + + # DO NOT try to compare fp.dtype and na.dtype, unless you have a lot of + # spare time to make sense of it and possibly chime in under #1908. def test_from_python(): From 1593ebc7beef22191c04c27c73f25e2b9154fd03 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 15:46:12 -0700 Subject: [PATCH 11/24] resolve clang-tidy warning --- tests/test_buffers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index c842f5fd52..daf36a794a 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -16,7 +16,7 @@ TEST_SUBMODULE(buffers, m) { m.def("format_descriptor_format", [](const std::string &cpp_name) { // https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables - static auto table = new std::map; + static auto *table = new std::map; if (table->empty()) { #define PYBIND11_ASSIGN_HELPER(...) \ (*table)[#__VA_ARGS__] = py::format_descriptor<__VA_ARGS__>::format(); From 3f04188baafa90c0cf386762c60dd74ddc20cc95 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 16:24:01 -0700 Subject: [PATCH 12/24] Account for np.float128, np.complex256 not being available on Windows, in a future-proof way. --- tests/test_buffers.py | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/tests/test_buffers.py b/tests/test_buffers.py index c2795e1b7a..c8fa697075 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -10,6 +10,14 @@ np = pytest.importorskip("numpy") +if env.WIN: + # Windows does not have these (see e.g. #1908). But who knows, maybe later? + np_float128_or_none = getattr(np, "float128", None) + np_complex256_or_none = getattr(np, "complex256", None) +else: + np_float128_or_none = np.float128 + np_complex256_or_none = np.complex256 + @pytest.mark.parametrize( ("cpp_name", "expected_fmts", "np_array_dtype"), @@ -26,10 +34,10 @@ ("std::uint64_t", ["Q"], np.uint64), ("float", ["f"], np.float32), ("double", ["d"], np.float64), - ("long double", ["g", "d"], np.float128), + ("long double", ["g", "d"], np_float128_or_none), ("std::complex", ["Zf"], np.complex64), ("std::complex", ["Zd"], np.complex128), - ("std::complex", ["Zg", "Zd"], np.complex256), + ("std::complex", ["Zg", "Zd"], np_complex256_or_none), ], ) def test_format_descriptor_format(cpp_name, expected_fmts, np_array_dtype): @@ -39,15 +47,16 @@ def test_format_descriptor_format(cpp_name, expected_fmts, np_array_dtype): # Everything below just documents long-standing inconsistencies. # See also: https://github.com/pybind/pybind11/issues/1908 - # py::format_descriptor<> vs np.array: - na = np.array([], dtype=np_array_dtype) - bi = m.get_buffer_info(na) - if fmt == "q": - assert bi.format in ["q", "l"] - elif fmt == "Q": - assert bi.format in ["Q", "L"] - else: - assert bi.format == fmt + if np_array_dtype is not None: + # py::format_descriptor<> vs np.array: + na = np.array([], dtype=np_array_dtype) + bi = m.get_buffer_info(na) + if fmt == "q": + assert bi.format in ["q", "l"] + elif fmt == "Q": + assert bi.format in ["Q", "L"] + else: + assert bi.format == fmt # py::format_descriptor<> vs np.format_parser(): fmtp = fmt[1:] if fmt.startswith("Z") else fmt From 38aa697d0daa567a1e1fa777ac09b370636345bc Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 16:49:18 -0700 Subject: [PATCH 13/24] Fully address i|q|l ambiguity (hopefully). --- tests/test_buffers.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_buffers.py b/tests/test_buffers.py index c8fa697075..5b69702069 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -51,10 +51,10 @@ def test_format_descriptor_format(cpp_name, expected_fmts, np_array_dtype): # py::format_descriptor<> vs np.array: na = np.array([], dtype=np_array_dtype) bi = m.get_buffer_info(na) - if fmt == "q": - assert bi.format in ["q", "l"] - elif fmt == "Q": - assert bi.format in ["Q", "L"] + if fmt in ("i", "q"): + assert bi.format in [fmt, "l"] + elif fmt in ("I", "Q"): + assert bi.format in [fmt, "L"] else: assert bi.format == fmt From 7f124bb5686037a3d28b511952ca24882d3e2f65 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 18:32:56 -0700 Subject: [PATCH 14/24] Remove the new `np.format_parser()`-based test, it's much more distracting than useful. --- tests/test_buffers.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/test_buffers.py b/tests/test_buffers.py index 5b69702069..a401e1f623 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -44,11 +44,7 @@ def test_format_descriptor_format(cpp_name, expected_fmts, np_array_dtype): fmt = m.format_descriptor_format(cpp_name) assert fmt in expected_fmts - # Everything below just documents long-standing inconsistencies. - # See also: https://github.com/pybind/pybind11/issues/1908 - if np_array_dtype is not None: - # py::format_descriptor<> vs np.array: na = np.array([], dtype=np_array_dtype) bi = m.get_buffer_info(na) if fmt in ("i", "q"): @@ -58,14 +54,6 @@ def test_format_descriptor_format(cpp_name, expected_fmts, np_array_dtype): else: assert bi.format == fmt - # py::format_descriptor<> vs np.format_parser(): - fmtp = fmt[1:] if fmt.startswith("Z") else fmt - fp = np.format_parser(fmtp, [], []) - assert fp.dtype is not None - - # DO NOT try to compare fp.dtype and na.dtype, unless you have a lot of - # spare time to make sense of it and possibly chime in under #1908. - def test_from_python(): with pytest.raises(RuntimeError) as excinfo: From d432ce75b3963cc759afa640f669157cf110b291 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 18:44:07 -0700 Subject: [PATCH 15/24] Use bi.itemsize to disambiguate "l" or "L" --- tests/test_buffers.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/test_buffers.py b/tests/test_buffers.py index a401e1f623..afa5fc571f 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -47,12 +47,12 @@ def test_format_descriptor_format(cpp_name, expected_fmts, np_array_dtype): if np_array_dtype is not None: na = np.array([], dtype=np_array_dtype) bi = m.get_buffer_info(na) - if fmt in ("i", "q"): - assert bi.format in [fmt, "l"] - elif fmt in ("I", "Q"): - assert bi.format in [fmt, "L"] - else: - assert bi.format == fmt + bif = bi.format + if bif == "l": + bif = "i" if bi.itemsize == 4 else "q" + elif bif == "L": + bif = "I" if bi.itemsize == 4 else "Q" + assert bif == fmt def test_from_python(): From 18e1bd2a8969a4cfaa41ba2faedff9d897d1fe15 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 23:09:31 -0700 Subject: [PATCH 16/24] Use `py::detail::compare_buffer_info::compare()` to validate the `format_descriptor::format()` strings. --- tests/test_buffers.cpp | 55 +++++++++++++++----------- tests/test_buffers.py | 90 ++++++++++++++++++++++-------------------- 2 files changed, 79 insertions(+), 66 deletions(-) diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index daf36a794a..ed9013ae7b 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -14,32 +14,39 @@ #include "pybind11_tests.h" TEST_SUBMODULE(buffers, m) { - m.def("format_descriptor_format", [](const std::string &cpp_name) { - // https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables - static auto *table = new std::map; - if (table->empty()) { + m.attr("std_is_same_double_long_double") = std::is_same::value; + + m.def("format_descriptor_format_compare", + [](const std::string &cpp_name, const py::buffer &buffer) { + // https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables + static auto *format_table = new std::map; + static auto *compare_table + = new std::map; + if (format_table->empty()) { #define PYBIND11_ASSIGN_HELPER(...) \ - (*table)[#__VA_ARGS__] = py::format_descriptor<__VA_ARGS__>::format(); - PYBIND11_ASSIGN_HELPER(PyObject *) - PYBIND11_ASSIGN_HELPER(bool) - PYBIND11_ASSIGN_HELPER(std::int8_t) - PYBIND11_ASSIGN_HELPER(std::uint8_t) - PYBIND11_ASSIGN_HELPER(std::int16_t) - PYBIND11_ASSIGN_HELPER(std::uint16_t) - PYBIND11_ASSIGN_HELPER(std::int32_t) - PYBIND11_ASSIGN_HELPER(std::uint32_t) - PYBIND11_ASSIGN_HELPER(std::int64_t) - PYBIND11_ASSIGN_HELPER(std::uint64_t) - PYBIND11_ASSIGN_HELPER(float) - PYBIND11_ASSIGN_HELPER(double) - PYBIND11_ASSIGN_HELPER(long double) - PYBIND11_ASSIGN_HELPER(std::complex) - PYBIND11_ASSIGN_HELPER(std::complex) - PYBIND11_ASSIGN_HELPER(std::complex) + (*format_table)[#__VA_ARGS__] = py::format_descriptor<__VA_ARGS__>::format(); \ + (*compare_table)[#__VA_ARGS__] = py::detail::compare_buffer_info<__VA_ARGS__>::compare; + PYBIND11_ASSIGN_HELPER(PyObject *) + PYBIND11_ASSIGN_HELPER(bool) + PYBIND11_ASSIGN_HELPER(std::int8_t) + PYBIND11_ASSIGN_HELPER(std::uint8_t) + PYBIND11_ASSIGN_HELPER(std::int16_t) + PYBIND11_ASSIGN_HELPER(std::uint16_t) + PYBIND11_ASSIGN_HELPER(std::int32_t) + PYBIND11_ASSIGN_HELPER(std::uint32_t) + PYBIND11_ASSIGN_HELPER(std::int64_t) + PYBIND11_ASSIGN_HELPER(std::uint64_t) + PYBIND11_ASSIGN_HELPER(float) + PYBIND11_ASSIGN_HELPER(double) + PYBIND11_ASSIGN_HELPER(long double) + PYBIND11_ASSIGN_HELPER(std::complex) + PYBIND11_ASSIGN_HELPER(std::complex) + PYBIND11_ASSIGN_HELPER(std::complex) #undef PYBIND11_ASSIGN_HELPER - } - return (*table)[cpp_name]; - }); + } + return std::pair((*format_table)[cpp_name], + (*compare_table)[cpp_name](buffer.request())); + }); // test_from_python / test_to_python: class Matrix { diff --git a/tests/test_buffers.py b/tests/test_buffers.py index afa5fc571f..108154231a 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -10,49 +10,55 @@ np = pytest.importorskip("numpy") -if env.WIN: - # Windows does not have these (see e.g. #1908). But who knows, maybe later? - np_float128_or_none = getattr(np, "float128", None) - np_complex256_or_none = getattr(np, "complex256", None) +if m.std_is_same_double_long_double: # Windows. + np_float128 = None + np_complex256 = None else: - np_float128_or_none = np.float128 - np_complex256_or_none = np.complex256 - - -@pytest.mark.parametrize( - ("cpp_name", "expected_fmts", "np_array_dtype"), - [ - ("PyObject *", ["O"], object), - ("bool", ["?"], np.bool_), - ("std::int8_t", ["b"], np.int8), - ("std::uint8_t", ["B"], np.uint8), - ("std::int16_t", ["h"], np.int16), - ("std::uint16_t", ["H"], np.uint16), - ("std::int32_t", ["i"], np.int32), - ("std::uint32_t", ["I"], np.uint32), - ("std::int64_t", ["q"], np.int64), - ("std::uint64_t", ["Q"], np.uint64), - ("float", ["f"], np.float32), - ("double", ["d"], np.float64), - ("long double", ["g", "d"], np_float128_or_none), - ("std::complex", ["Zf"], np.complex64), - ("std::complex", ["Zd"], np.complex128), - ("std::complex", ["Zg", "Zd"], np_complex256_or_none), - ], -) -def test_format_descriptor_format(cpp_name, expected_fmts, np_array_dtype): - fmt = m.format_descriptor_format(cpp_name) - assert fmt in expected_fmts - - if np_array_dtype is not None: - na = np.array([], dtype=np_array_dtype) - bi = m.get_buffer_info(na) - bif = bi.format - if bif == "l": - bif = "i" if bi.itemsize == 4 else "q" - elif bif == "L": - bif = "I" if bi.itemsize == 4 else "Q" - assert bif == fmt + np_float128 = np.float128 + np_complex256 = np.complex256 + +CPP_NAME_FORMAT_NP_DTYPE_TABLE = [ + item + for item in [ + ("PyObject *", "O", object), + ("bool", "?", np.bool_), + ("std::int8_t", "b", np.int8), + ("std::uint8_t", "B", np.uint8), + ("std::int16_t", "h", np.int16), + ("std::uint16_t", "H", np.uint16), + ("std::int32_t", "i", np.int32), + ("std::uint32_t", "I", np.uint32), + ("std::int64_t", "q", np.int64), + ("std::uint64_t", "Q", np.uint64), + ("float", "f", np.float32), + ("double", "d", np.float64), + ("long double", "g", np_float128), + ("std::complex", "Zf", np.complex64), + ("std::complex", "Zd", np.complex128), + ("std::complex", "Zg", np_complex256), + ] + if item[-1] is not None +] +CPP_NAME_FORMAT_TABLE = [ + (cpp_name, format) for cpp_name, format, _ in CPP_NAME_FORMAT_NP_DTYPE_TABLE +] +CPP_NAME_NP_DTYPE_TABLE = [ + (cpp_name, np_dtype) for cpp_name, _, np_dtype in CPP_NAME_FORMAT_NP_DTYPE_TABLE +] + + +@pytest.mark.parametrize(("cpp_name", "np_dtype"), CPP_NAME_NP_DTYPE_TABLE) +def test_format_descriptor_format_compare(cpp_name, np_dtype): + np_array = np.array([], dtype=np_dtype) + for other_cpp_name, expected_format in CPP_NAME_FORMAT_TABLE: + format, np_array_is_matching = m.format_descriptor_format_compare( + other_cpp_name, np_array + ) + assert format == expected_format + if other_cpp_name == cpp_name: + assert np_array_is_matching + else: + assert not np_array_is_matching def test_from_python(): From 029b157540f18b81bad38c92d666b6414b3ef547 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 18 May 2023 23:28:53 -0700 Subject: [PATCH 17/24] Add `buffer_info::compare` to make `detail::compare_buffer_info::compare` more visible & accessible. --- include/pybind11/buffer_info.h | 10 +++++++++- tests/test_buffers.cpp | 4 ++-- tests/test_buffers.py | 4 ++-- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/include/pybind11/buffer_info.h b/include/pybind11/buffer_info.h index 06120d5563..f8689babf3 100644 --- a/include/pybind11/buffer_info.h +++ b/include/pybind11/buffer_info.h @@ -37,6 +37,9 @@ inline std::vector f_strides(const std::vector &shape, ssize_t return strides; } +template +struct compare_buffer_info; + PYBIND11_NAMESPACE_END(detail) /// Information record describing a Python buffer object @@ -150,6 +153,11 @@ struct buffer_info { Py_buffer *view() const { return m_view; } Py_buffer *&view() { return m_view; } + template + static bool compare(const buffer_info &b) { + return detail::compare_buffer_info::compare(b); + } + private: struct private_ctr_tag {}; @@ -170,7 +178,7 @@ struct buffer_info { PYBIND11_NAMESPACE_BEGIN(detail) -template +template struct compare_buffer_info { static bool compare(const buffer_info &b) { return b.format == format_descriptor::format() && b.itemsize == (ssize_t) sizeof(T); diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index ed9013ae7b..ab727552f4 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -16,7 +16,7 @@ TEST_SUBMODULE(buffers, m) { m.attr("std_is_same_double_long_double") = std::is_same::value; - m.def("format_descriptor_format_compare", + m.def("format_descriptor_format_buffer_info_compare", [](const std::string &cpp_name, const py::buffer &buffer) { // https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables static auto *format_table = new std::map; @@ -25,7 +25,7 @@ TEST_SUBMODULE(buffers, m) { if (format_table->empty()) { #define PYBIND11_ASSIGN_HELPER(...) \ (*format_table)[#__VA_ARGS__] = py::format_descriptor<__VA_ARGS__>::format(); \ - (*compare_table)[#__VA_ARGS__] = py::detail::compare_buffer_info<__VA_ARGS__>::compare; + (*compare_table)[#__VA_ARGS__] = py::buffer_info::compare<__VA_ARGS__>; PYBIND11_ASSIGN_HELPER(PyObject *) PYBIND11_ASSIGN_HELPER(bool) PYBIND11_ASSIGN_HELPER(std::int8_t) diff --git a/tests/test_buffers.py b/tests/test_buffers.py index 108154231a..4a60ea4261 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -48,10 +48,10 @@ @pytest.mark.parametrize(("cpp_name", "np_dtype"), CPP_NAME_NP_DTYPE_TABLE) -def test_format_descriptor_format_compare(cpp_name, np_dtype): +def test_format_descriptor_format_buffer_info_compare(cpp_name, np_dtype): np_array = np.array([], dtype=np_dtype) for other_cpp_name, expected_format in CPP_NAME_FORMAT_TABLE: - format, np_array_is_matching = m.format_descriptor_format_compare( + format, np_array_is_matching = m.format_descriptor_format_buffer_info_compare( other_cpp_name, np_array ) assert format == expected_format From d9e3bd3248dba9c4783b41d02ccf9b4000546b5b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 19 May 2023 00:04:11 -0700 Subject: [PATCH 18/24] silence clang-tidy warning --- include/pybind11/buffer_info.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/pybind11/buffer_info.h b/include/pybind11/buffer_info.h index f8689babf3..403034ed93 100644 --- a/include/pybind11/buffer_info.h +++ b/include/pybind11/buffer_info.h @@ -181,6 +181,7 @@ PYBIND11_NAMESPACE_BEGIN(detail) template struct compare_buffer_info { static bool compare(const buffer_info &b) { + // NOLINTNEXTLINE(bugprone-sizeof-expression) Needed for `PyObject *` return b.format == format_descriptor::format() && b.itemsize == (ssize_t) sizeof(T); } }; From e9a289c50fc07199806d14ded644215ab6f03afa Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 19 May 2023 00:17:30 -0700 Subject: [PATCH 19/24] pytest-compatible access to np.float128, np.complex256 --- tests/test_buffers.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/tests/test_buffers.py b/tests/test_buffers.py index 4a60ea4261..62c3efd388 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -10,12 +10,13 @@ np = pytest.importorskip("numpy") -if m.std_is_same_double_long_double: # Windows. - np_float128 = None - np_complex256 = None -else: - np_float128 = np.float128 - np_complex256 = np.complex256 + +def np_dtype_long_double_or_none(name): + # Intentionally not using getattr(np, name, None), to be strict. + if m.std_is_same_double_long_double: # Windows. + return None + return getattr(np, name) + CPP_NAME_FORMAT_NP_DTYPE_TABLE = [ item @@ -32,10 +33,10 @@ ("std::uint64_t", "Q", np.uint64), ("float", "f", np.float32), ("double", "d", np.float64), - ("long double", "g", np_float128), + ("long double", "g", np_dtype_long_double_or_none("float128")), ("std::complex", "Zf", np.complex64), ("std::complex", "Zd", np.complex128), - ("std::complex", "Zg", np_complex256), + ("std::complex", "Zg", np_dtype_long_double_or_none("complex256")), ] if item[-1] is not None ] From 8abe0e922ce372bb64591758c114fd95de11aab9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 19 May 2023 00:34:53 -0700 Subject: [PATCH 20/24] Revert "pytest-compatible access to np.float128, np.complex256" This reverts commit e9a289c50fc07199806d14ded644215ab6f03afa. --- tests/test_buffers.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/tests/test_buffers.py b/tests/test_buffers.py index 62c3efd388..4a60ea4261 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -10,13 +10,12 @@ np = pytest.importorskip("numpy") - -def np_dtype_long_double_or_none(name): - # Intentionally not using getattr(np, name, None), to be strict. - if m.std_is_same_double_long_double: # Windows. - return None - return getattr(np, name) - +if m.std_is_same_double_long_double: # Windows. + np_float128 = None + np_complex256 = None +else: + np_float128 = np.float128 + np_complex256 = np.complex256 CPP_NAME_FORMAT_NP_DTYPE_TABLE = [ item @@ -33,10 +32,10 @@ def np_dtype_long_double_or_none(name): ("std::uint64_t", "Q", np.uint64), ("float", "f", np.float32), ("double", "d", np.float64), - ("long double", "g", np_dtype_long_double_or_none("float128")), + ("long double", "g", np_float128), ("std::complex", "Zf", np.complex64), ("std::complex", "Zd", np.complex128), - ("std::complex", "Zg", np_dtype_long_double_or_none("complex256")), + ("std::complex", "Zg", np_complex256), ] if item[-1] is not None ] From b09e75b25c91bd300a1f2dbaabb1056a58d1d942 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 19 May 2023 00:39:17 -0700 Subject: [PATCH 21/24] Use `sizeof(long double) == sizeof(double)` instead of `std::is_same<>` --- tests/test_buffers.cpp | 2 +- tests/test_buffers.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index ab727552f4..879ff0eae3 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -14,7 +14,7 @@ #include "pybind11_tests.h" TEST_SUBMODULE(buffers, m) { - m.attr("std_is_same_double_long_double") = std::is_same::value; + m.attr("long_double_and_double_have_same_size") = (sizeof(long double) == sizeof(double)); m.def("format_descriptor_format_buffer_info_compare", [](const std::string &cpp_name, const py::buffer &buffer) { diff --git a/tests/test_buffers.py b/tests/test_buffers.py index 4a60ea4261..8014c38e6c 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -10,7 +10,7 @@ np = pytest.importorskip("numpy") -if m.std_is_same_double_long_double: # Windows. +if m.long_double_and_double_have_same_size: # Windows. np_float128 = None np_complex256 = None else: From ba7063e813c22b510b40859910a1fa97d637a054 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 19 May 2023 01:30:22 -0700 Subject: [PATCH 22/24] Report skipped `long double` tests. --- tests/test_buffers.py | 55 ++++++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/tests/test_buffers.py b/tests/test_buffers.py index 8014c38e6c..3aba8019d8 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -10,37 +10,38 @@ np = pytest.importorskip("numpy") -if m.long_double_and_double_have_same_size: # Windows. +if m.long_double_and_double_have_same_size: + # Determined by the compiler used to build the pybind11 tests + # (e.g. MSVC gets here, but MinGW might not). np_float128 = None np_complex256 = None else: - np_float128 = np.float128 - np_complex256 = np.complex256 + # Determined by the compiler used to build numpy (e.g. MinGW). + np_float128 = getattr(np, *["float128"] * 2) + np_complex256 = getattr(np, *["complex256"] * 2) CPP_NAME_FORMAT_NP_DTYPE_TABLE = [ - item - for item in [ - ("PyObject *", "O", object), - ("bool", "?", np.bool_), - ("std::int8_t", "b", np.int8), - ("std::uint8_t", "B", np.uint8), - ("std::int16_t", "h", np.int16), - ("std::uint16_t", "H", np.uint16), - ("std::int32_t", "i", np.int32), - ("std::uint32_t", "I", np.uint32), - ("std::int64_t", "q", np.int64), - ("std::uint64_t", "Q", np.uint64), - ("float", "f", np.float32), - ("double", "d", np.float64), - ("long double", "g", np_float128), - ("std::complex", "Zf", np.complex64), - ("std::complex", "Zd", np.complex128), - ("std::complex", "Zg", np_complex256), - ] - if item[-1] is not None + ("PyObject *", "O", object), + ("bool", "?", np.bool_), + ("std::int8_t", "b", np.int8), + ("std::uint8_t", "B", np.uint8), + ("std::int16_t", "h", np.int16), + ("std::uint16_t", "H", np.uint16), + ("std::int32_t", "i", np.int32), + ("std::uint32_t", "I", np.uint32), + ("std::int64_t", "q", np.int64), + ("std::uint64_t", "Q", np.uint64), + ("float", "f", np.float32), + ("double", "d", np.float64), + ("long double", "g", np_float128), + ("std::complex", "Zf", np.complex64), + ("std::complex", "Zd", np.complex128), + ("std::complex", "Zg", np_complex256), ] CPP_NAME_FORMAT_TABLE = [ - (cpp_name, format) for cpp_name, format, _ in CPP_NAME_FORMAT_NP_DTYPE_TABLE + (cpp_name, format) + for cpp_name, format, np_dtype in CPP_NAME_FORMAT_NP_DTYPE_TABLE + if np_dtype is not None ] CPP_NAME_NP_DTYPE_TABLE = [ (cpp_name, np_dtype) for cpp_name, _, np_dtype in CPP_NAME_FORMAT_NP_DTYPE_TABLE @@ -49,6 +50,12 @@ @pytest.mark.parametrize(("cpp_name", "np_dtype"), CPP_NAME_NP_DTYPE_TABLE) def test_format_descriptor_format_buffer_info_compare(cpp_name, np_dtype): + if np_dtype is None: + pytest.skip( + f"cpp_name=`{cpp_name}`: `long double` and `double` have same size." + ) + if isinstance(np_dtype, str): + pytest.skip(f"np.{np_dtype} does not exist.") np_array = np.array([], dtype=np_dtype) for other_cpp_name, expected_format in CPP_NAME_FORMAT_TABLE: format, np_array_is_matching = m.format_descriptor_format_buffer_info_compare( From a4d61b455f6a959d3cc5a98eaca46e14b27195c9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 19 May 2023 11:01:06 -0700 Subject: [PATCH 23/24] Change the name of the new `buffer_info` member function to `item_type_is_equivalent_to`. Add comment defining "equivalent" by example. --- include/pybind11/buffer_info.h | 8 +++++++- tests/test_buffers.cpp | 8 ++++---- tests/test_buffers.py | 4 ++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/include/pybind11/buffer_info.h b/include/pybind11/buffer_info.h index 403034ed93..8722d697ca 100644 --- a/include/pybind11/buffer_info.h +++ b/include/pybind11/buffer_info.h @@ -153,8 +153,14 @@ struct buffer_info { Py_buffer *view() const { return m_view; } Py_buffer *&view() { return m_view; } + /* True if the buffer item type is equivalent to `T`. */ + // To define "equivalent" by example: + // `buffer_info::item_type_is_equivalent_to(b)` and + // `buffer_info::item_type_is_equivalent_to(b)` may both be true + // on some platforms, but `int` and `unsigned` will never be equivalent. + // For the ground truth, please inspect `detail::compare_buffer_info<>`. template - static bool compare(const buffer_info &b) { + static bool item_type_is_equivalent_to(const buffer_info &b) { return detail::compare_buffer_info::compare(b); } diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index 879ff0eae3..1169891f23 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -16,16 +16,16 @@ TEST_SUBMODULE(buffers, m) { m.attr("long_double_and_double_have_same_size") = (sizeof(long double) == sizeof(double)); - m.def("format_descriptor_format_buffer_info_compare", + m.def("format_descriptor_format_buffer_info_equiv", [](const std::string &cpp_name, const py::buffer &buffer) { // https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables static auto *format_table = new std::map; - static auto *compare_table + static auto *equiv_table = new std::map; if (format_table->empty()) { #define PYBIND11_ASSIGN_HELPER(...) \ (*format_table)[#__VA_ARGS__] = py::format_descriptor<__VA_ARGS__>::format(); \ - (*compare_table)[#__VA_ARGS__] = py::buffer_info::compare<__VA_ARGS__>; + (*equiv_table)[#__VA_ARGS__] = py::buffer_info::item_type_is_equivalent_to<__VA_ARGS__>; PYBIND11_ASSIGN_HELPER(PyObject *) PYBIND11_ASSIGN_HELPER(bool) PYBIND11_ASSIGN_HELPER(std::int8_t) @@ -45,7 +45,7 @@ TEST_SUBMODULE(buffers, m) { #undef PYBIND11_ASSIGN_HELPER } return std::pair((*format_table)[cpp_name], - (*compare_table)[cpp_name](buffer.request())); + (*equiv_table)[cpp_name](buffer.request())); }); // test_from_python / test_to_python: diff --git a/tests/test_buffers.py b/tests/test_buffers.py index 3aba8019d8..63d9d869fd 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -49,7 +49,7 @@ @pytest.mark.parametrize(("cpp_name", "np_dtype"), CPP_NAME_NP_DTYPE_TABLE) -def test_format_descriptor_format_buffer_info_compare(cpp_name, np_dtype): +def test_format_descriptor_format_buffer_info_equiv(cpp_name, np_dtype): if np_dtype is None: pytest.skip( f"cpp_name=`{cpp_name}`: `long double` and `double` have same size." @@ -58,7 +58,7 @@ def test_format_descriptor_format_buffer_info_compare(cpp_name, np_dtype): pytest.skip(f"np.{np_dtype} does not exist.") np_array = np.array([], dtype=np_dtype) for other_cpp_name, expected_format in CPP_NAME_FORMAT_TABLE: - format, np_array_is_matching = m.format_descriptor_format_buffer_info_compare( + format, np_array_is_matching = m.format_descriptor_format_buffer_info_equiv( other_cpp_name, np_array ) assert format == expected_format From ef34d293495844933b1d936a058c5030c31f3ad3 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 19 May 2023 17:45:58 -0700 Subject: [PATCH 24/24] Change `item_type_is_equivalent_to<>()` from `static` function to member function, as suggested by @Lalaland --- include/pybind11/buffer_info.h | 4 ++-- tests/test_buffers.cpp | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/pybind11/buffer_info.h b/include/pybind11/buffer_info.h index 8722d697ca..b99ee8bef4 100644 --- a/include/pybind11/buffer_info.h +++ b/include/pybind11/buffer_info.h @@ -160,8 +160,8 @@ struct buffer_info { // on some platforms, but `int` and `unsigned` will never be equivalent. // For the ground truth, please inspect `detail::compare_buffer_info<>`. template - static bool item_type_is_equivalent_to(const buffer_info &b) { - return detail::compare_buffer_info::compare(b); + bool item_type_is_equivalent_to() const { + return detail::compare_buffer_info::compare(*this); } private: diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index 1169891f23..b5b8c355b3 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -21,11 +21,11 @@ TEST_SUBMODULE(buffers, m) { // https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables static auto *format_table = new std::map; static auto *equiv_table - = new std::map; + = new std::map; if (format_table->empty()) { #define PYBIND11_ASSIGN_HELPER(...) \ (*format_table)[#__VA_ARGS__] = py::format_descriptor<__VA_ARGS__>::format(); \ - (*equiv_table)[#__VA_ARGS__] = py::buffer_info::item_type_is_equivalent_to<__VA_ARGS__>; + (*equiv_table)[#__VA_ARGS__] = &py::buffer_info::item_type_is_equivalent_to<__VA_ARGS__>; PYBIND11_ASSIGN_HELPER(PyObject *) PYBIND11_ASSIGN_HELPER(bool) PYBIND11_ASSIGN_HELPER(std::int8_t) @@ -44,8 +44,8 @@ TEST_SUBMODULE(buffers, m) { PYBIND11_ASSIGN_HELPER(std::complex) #undef PYBIND11_ASSIGN_HELPER } - return std::pair((*format_table)[cpp_name], - (*equiv_table)[cpp_name](buffer.request())); + return std::pair( + (*format_table)[cpp_name], (buffer.request().*((*equiv_table)[cpp_name]))()); }); // test_from_python / test_to_python: