-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Changes from 8 commits
5168c13
d53a796
5bea2a8
50eaa3a
82ce80f
20b9baf
0640eb3
ddb625e
03dafde
28492ed
1593ebc
3f04188
38aa697
7f124bb
d432ce7
18e1bd2
029b157
d9e3bd3
e9a289c
8abe0e9
b09e75b
ba7063e
a4d61b4
ef34d29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -287,6 +287,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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Cons: 2-3 x 2 more lines of code. Pros: The In case a |
||
PYBIND11_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); | ||
using props = EigenProps<Type>; | ||
|
||
bool load(handle src, bool convert) { | ||
|
@@ -405,6 +407,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_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); | ||
|
||
private: | ||
using props = EigenProps<MapType>; | ||
|
||
|
@@ -457,6 +462,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_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); | ||
using MapType = Eigen::Map<PlainObjectType, 0, StrideType>; | ||
using Array | ||
= array_t<Scalar, | ||
|
@@ -604,6 +611,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_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); | ||
|
||
protected: | ||
using Matrix | ||
= Eigen::Matrix<typename Type::Scalar, Type::RowsAtCompileTime, Type::ColsAtCompileTime>; | ||
|
@@ -632,6 +642,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_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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,6 +164,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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above. |
||
PYBIND11_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); | ||
|
@@ -359,6 +361,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_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); | ||
using MapType = Eigen::TensorMap<Type, Options>; | ||
using Helper = eigen_tensor_helper<remove_cv_t<Type>>; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,12 +7,40 @@ | |
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) { | ||
|
||
#define PYBIND11_LOCAL_DEF(...) \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if it's worth changing, but I tend to not like macros that return values. I would have written this as:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed. |
||
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<float>) | ||
PYBIND11_LOCAL_DEF(std::complex<double>) | ||
PYBIND11_LOCAL_DEF(std::complex<long double>) | ||
return std::string("UNKNOWN"); | ||
}); | ||
|
||
#undef PYBIND11_LOCAL_DEF | ||
|
||
// test_from_python / test_to_python: | ||
class Matrix { | ||
public: | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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<float>", ["Zf"]), | ||||||||||||||||||||||||||||||||||||||
("std::complex<double>", ["Zd"]), | ||||||||||||||||||||||||||||||||||||||
("std::complex<long double>", ["Zg", "Zd"]), | ||||||||||||||||||||||||||||||||||||||
("", ["UNKNOWN"]), | ||||||||||||||||||||||||||||||||||||||
], | ||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||
def test_format_descriptor_format(cpp_name, expected_codes): | ||||||||||||||||||||||||||||||||||||||
assert m.format_descriptor_format(cpp_name) in expected_codes | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add an assert that the format descriptor is a valid numpy format descriptor https://numpy.org/doc/stable/reference/generated/numpy.format_parser.html#numpy-format-parser assert np. format_parser(blah).dtype is not None There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done-ish. See my comments in the test code. What a lucky man I was to never have seen #1908 before. I see Windows isn't happy about my latest version of the test. Fixing... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@lalaland Thanks a lot for nudging me in that direction! Using pybind11/include/pybind11/buffer_info.h Lines 173 to 190 in d72ffb4
From there it was only a small step to add a public (not in The new public interface is also exactly what I need for a clean solution here: |
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
def test_from_python(): | ||||||||||||||||||||||||||||||||||||||
with pytest.raises(RuntimeError) as excinfo: | ||||||||||||||||||||||||||||||||||||||
m.Matrix(np.array([1, 2, 3])) # trying to assign a 1D array | ||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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")); | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: pybind11/include/pybind11/cast.h Lines 1062 to 1078 in d72ffb4
|
||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||
return arr_from_list; | ||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
sm.def("return_array_pyobject_ptr_from_list", | ||||||||||||||||||||||||||||||||||||
[](const py::list &objs) -> py::array_t<PyObject *> { return objs; }); | ||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -595,3 +595,74 @@ def test_round_trip_float(): | |
arr = np.zeros((), np.float64) | ||
arr[()] = 37.2 | ||
assert m.round_trip_float(arr) == 37.2 | ||
|
||
|
||
# HINT: An easy and robust way (although only manual unfortunately) to check for | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be possible to do this automatically using weakrefs: https://docs.python.org/3/library/weakref.html#weakref.ref I'm not sure it's worth changing for this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, but: That only works when making strong assumptions about the reference-counting and garbage collection behavior. In practice, when targeting C Python only, that's usually fine. Fundamentally though such tests are creating tech debt that might get in the way of core interpreter developments in the future. Therefore I generally write tests involving What would be great to have: a pytest feature, e.g. |
||
# 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 | ||
|
||
|
||
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this message Eigen specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I introduced pybind11/eigen/common.h to have central location for the message. (Previously I shied away from doing that, that's why the message wasn't specific. But that was really just taking a shortcut. What we have now this is definitely better.)