Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add format_descriptor<> & npy_format_descriptor<> PyObject * specializations. #4674

Merged
merged 24 commits into from
May 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
5168c13
Add `npy_format_descriptor<PyObject *>` to enable `py::array_t<PyObje…
May 17, 2023
d53a796
resolve clang-tidy warning
May 17, 2023
5bea2a8
Use existing constructor instead of adding a static method. Thanks @S…
May 17, 2023
50eaa3a
Add `format_descriptor<PyObject *>`
May 17, 2023
82ce80f
Add test_format_descriptor_format
May 18, 2023
20b9baf
Ensure the Eigen `type_caster`s do not segfault when loading arrays w…
May 18, 2023
0640eb3
Use `static_assert()` `!std::is_pointer<>` to replace runtime guards.
May 18, 2023
ddb625e
Add comments to explain how to check for ref-count bugs. (NO code cha…
May 18, 2023
03dafde
Make the "Pointer types ... are not supported" message Eigen-specific…
May 18, 2023
28492ed
Change "format_descriptor_format" implementation as suggested by @Lal…
May 18, 2023
1593ebc
resolve clang-tidy warning
May 18, 2023
3f04188
Account for np.float128, np.complex256 not being available on Windows…
May 18, 2023
38aa697
Fully address i|q|l ambiguity (hopefully).
May 18, 2023
7f124bb
Remove the new `np.format_parser()`-based test, it's much more distra…
May 19, 2023
d432ce7
Use bi.itemsize to disambiguate "l" or "L"
May 19, 2023
18e1bd2
Use `py::detail::compare_buffer_info<T>::compare()` to validate the `…
May 19, 2023
029b157
Add `buffer_info::compare<T>` to make `detail::compare_buffer_info<T>…
May 19, 2023
d9e3bd3
silence clang-tidy warning
May 19, 2023
e9a289c
pytest-compatible access to np.float128, np.complex256
May 19, 2023
8abe0e9
Revert "pytest-compatible access to np.float128, np.complex256"
May 19, 2023
b09e75b
Use `sizeof(long double) == sizeof(double)` instead of `std::is_same<>`
May 19, 2023
ba7063e
Report skipped `long double` tests.
May 19, 2023
a4d61b4
Change the name of the new `buffer_info` member function to `item_typ…
May 19, 2023
ef34d29
Change `item_type_is_equivalent_to<>()` from `static` function to mem…
May 20, 2023
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
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 16 additions & 1 deletion include/pybind11/buffer_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ inline std::vector<ssize_t> f_strides(const std::vector<ssize_t> &shape, ssize_t
return strides;
}

template <typename T, typename SFINAE = void>
struct compare_buffer_info;

PYBIND11_NAMESPACE_END(detail)

/// Information record describing a Python buffer object
Expand Down Expand Up @@ -150,6 +153,17 @@ 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<int>(b)` and
// `buffer_info::item_type_is_equivalent_to<long>(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 <typename T>
bool item_type_is_equivalent_to() const {
return detail::compare_buffer_info<T>::compare(*this);
}

private:
struct private_ctr_tag {};

Expand All @@ -170,9 +184,10 @@ struct buffer_info {

PYBIND11_NAMESPACE_BEGIN(detail)

template <typename T, typename SFINAE = void>
template <typename T, typename SFINAE>
struct compare_buffer_info {
static bool compare(const buffer_info &b) {
// NOLINTNEXTLINE(bugprone-sizeof-expression) Needed for `PyObject *`
return b.format == format_descriptor<T>::format() && b.itemsize == (ssize_t) sizeof(T);
}
};
Expand Down
9 changes: 9 additions & 0 deletions include/pybind11/detail/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,15 @@ PYBIND11_RUNTIME_EXCEPTION(reference_cast_error, PyExc_RuntimeError) /// Used in
template <typename T, typename SFINAE = void>
struct format_descriptor {};

template <typename T>
struct format_descriptor<
T,
detail::enable_if_t<detail::is_same_ignoring_cvref<T, PyObject *>::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;
Expand Down
9 changes: 9 additions & 0 deletions include/pybind11/eigen/common.h
Original file line number Diff line number Diff line change
@@ -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."
13 changes: 13 additions & 0 deletions include/pybind11/eigen/matrix.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -287,6 +288,8 @@ handle eigen_encapsulate(Type *src) {
template <typename Type>
struct type_caster<Type, enable_if_t<is_eigen_dense_plain<Type>::value>> {
using Scalar = typename Type::Scalar;
static_assert(!std::is_pointer<Scalar>::value,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you might be able to move these asserts to EigenProps, which would reduce the amount of redundant code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was ambivalent before and still am a little bit, but decided to keep the 5 static_assert():

Cons: 2-3 x 2 more lines of code.

Pros:

The static_assert()s appear at or near the top of each of the type_casters, which makes them essentially a kind of documentation.

In case a static_assert() fails, it will be much more obvious that the message we want to send is: the type_caster does not support pointer types as scalar types.

PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED);
using props = EigenProps<Type>;

bool load(handle src, bool convert) {
Expand Down Expand Up @@ -405,6 +408,9 @@ struct type_caster<Type, enable_if_t<is_eigen_dense_plain<Type>::value>> {
// Base class for casting reference/map/block/etc. objects back to python.
template <typename MapType>
struct eigen_map_caster {
static_assert(!std::is_pointer<typename MapType::Scalar>::value,
PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED);

private:
using props = EigenProps<MapType>;

Expand Down Expand Up @@ -457,6 +463,8 @@ struct type_caster<
using Type = Eigen::Ref<PlainObjectType, 0, StrideType>;
using props = EigenProps<Type>;
using Scalar = typename props::Scalar;
static_assert(!std::is_pointer<Scalar>::value,
PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED);
using MapType = Eigen::Map<PlainObjectType, 0, StrideType>;
using Array
= array_t<Scalar,
Expand Down Expand Up @@ -604,6 +612,9 @@ struct type_caster<
// regular Eigen::Matrix, then casting that.
template <typename Type>
struct type_caster<Type, enable_if_t<is_eigen_other<Type>::value>> {
static_assert(!std::is_pointer<typename Type::Scalar>::value,
PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED);

protected:
using Matrix
= Eigen::Matrix<typename Type::Scalar, Type::RowsAtCompileTime, Type::ColsAtCompileTime>;
Expand Down Expand Up @@ -632,6 +643,8 @@ struct type_caster<Type, enable_if_t<is_eigen_other<Type>::value>> {
template <typename Type>
struct type_caster<Type, enable_if_t<is_eigen_sparse<Type>::value>> {
using Scalar = typename Type::Scalar;
static_assert(!std::is_pointer<Scalar>::value,
PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED);
using StorageIndex = remove_reference_t<decltype(*std::declval<Type>().outerIndexPtr())>;
using Index = typename Type::Index;
static constexpr bool rowMajor = Type::IsRowMajor;
Expand Down
5 changes: 5 additions & 0 deletions include/pybind11/eigen/tensor.h
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -164,6 +165,8 @@ PYBIND11_WARNING_POP

template <typename Type>
struct type_caster<Type, typename eigen_tensor_helper<Type>::ValidType> {
static_assert(!std::is_pointer<typename Type::Scalar>::value,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to not add the assert to eigen_tensor_helper instead to avoid duplicate lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See above.

PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED);
using Helper = eigen_tensor_helper<Type>;
static constexpr auto temp_name = get_tensor_descriptor<Type, false>::value;
PYBIND11_TYPE_CASTER(Type, temp_name);
Expand Down Expand Up @@ -359,6 +362,8 @@ struct get_storage_pointer_type<MapType, void_t<typename MapType::PointerArgType
template <typename Type, int Options>
struct type_caster<Eigen::TensorMap<Type, Options>,
typename eigen_tensor_helper<remove_cv_t<Type>>::ValidType> {
static_assert(!std::is_pointer<typename Type::Scalar>::value,
PYBIND11_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED);
using MapType = Eigen::TensorMap<Type, Options>;
using Helper = eigen_tensor_helper<remove_cv_t<Type>>;

Expand Down
18 changes: 12 additions & 6 deletions include/pybind11/numpy.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -1283,12 +1285,16 @@ struct npy_format_descriptor<
public:
static constexpr int value = values[detail::is_fmt_numeric<T>::index];

static pybind11::dtype dtype() {
if (auto *ptr = npy_api::get().PyArray_DescrFromType_(value)) {
return reinterpret_steal<pybind11::dtype>(ptr);
}
pybind11_fail("Unsupported buffer format!");
}
static pybind11::dtype dtype() { return pybind11::dtype(/*typenum*/ value); }
};

template <typename T>
struct npy_format_descriptor<T, enable_if_t<is_same_ignoring_cvref<T, PyObject *>::value>> {
static constexpr auto name = const_name("object");

static constexpr int value = npy_api::NPY_OBJECT_;

static pybind11::dtype dtype() { return pybind11::dtype(/*typenum*/ value); }
};

#define PYBIND11_DECL_CHAR_FMT \
Expand Down
1 change: 1 addition & 0 deletions tests/extra_python_package/test_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
}

eigen_headers = {
"include/pybind11/eigen/common.h",
"include/pybind11/eigen/matrix.h",
"include/pybind11/eigen/tensor.h",
}
Expand Down
35 changes: 35 additions & 0 deletions tests/test_buffers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,47 @@
BSD-style license that can be found in the LICENSE file.
*/

#include <pybind11/complex.h>
#include <pybind11/stl.h>

#include "constructor_stats.h"
#include "pybind11_tests.h"

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_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<std::string, std::string>;
static auto *equiv_table
= new std::map<std::string, bool (py::buffer_info::*)() const>;
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__>;
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<float>)
PYBIND11_ASSIGN_HELPER(std::complex<double>)
PYBIND11_ASSIGN_HELPER(std::complex<long double>)
#undef PYBIND11_ASSIGN_HELPER
}
return std::pair<std::string, bool>(
(*format_table)[cpp_name], (buffer.request().*((*equiv_table)[cpp_name]))());
});

// test_from_python / test_to_python:
class Matrix {
public:
Expand Down
57 changes: 57 additions & 0 deletions tests/test_buffers.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,63 @@

np = pytest.importorskip("numpy")

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:
# 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 = [
("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<float>", "Zf", np.complex64),
("std::complex<double>", "Zd", np.complex128),
("std::complex<long double>", "Zg", np_complex256),
]
CPP_NAME_FORMAT_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
]


@pytest.mark.parametrize(("cpp_name", "np_dtype"), CPP_NAME_NP_DTYPE_TABLE)
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."
)
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_equiv(
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():
with pytest.raises(RuntimeError) as excinfo:
Expand Down
26 changes: 26 additions & 0 deletions tests/test_numpy_array.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -523,4 +523,30 @@ TEST_SUBMODULE(numpy_array, sm) {
sm.def("test_fmt_desc_const_double", [](const py::array_t<const double> &) {});

sm.def("round_trip_float", [](double d) { return d; });

sm.def("pass_array_pyobject_ptr_return_sum_str_values",
[](const py::array_t<PyObject *> &objs) {
std::string sum_str_values;
for (const 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<PyObject *> &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<PyObject *> arr_from_list(static_cast<py::ssize_t>(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<PyObject *>(objs[i].attr("value"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be a silly question, but does this appropriately increase the reference count?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a silly question, this was something I was struggling with quite a bit:

// Note that `cast<PyObject *>(obj)` increments the reference count of `obj`.
// This is necessary for the case that `obj` is a temporary, and could
// not possibly be different, given
// 1. the established convention that the passed `handle` is borrowed, and
// 2. we don't want to force all generic code using `cast<T>()` to special-case
// handling of `T` = `PyObject *` (to increment the reference count there).
// It is the responsibility of the caller to ensure that the reference count
// is decremented.
template <typename T,
typename Handle,
detail::enable_if_t<detail::is_same_ignoring_cvref<T, PyObject *>::value
&& detail::is_same_ignoring_cvref<Handle, handle>::value,
int>
= 0>
T cast(Handle &&handle) {
return handle.inc_ref().ptr();
}

}
return arr_from_list;
});

sm.def("return_array_pyobject_ptr_from_list",
[](const py::list &objs) -> py::array_t<PyObject *> { return objs; });
}
Loading