-
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 22 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 |
---|---|---|
@@ -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." |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: | ||
|
@@ -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, | ||
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_EIGEN_MESSAGE_POINTER_TYPES_ARE_NOT_SUPPORTED); | ||
using props = EigenProps<Type>; | ||
|
||
bool load(handle src, bool convert) { | ||
|
@@ -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>; | ||
|
||
|
@@ -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, | ||
|
@@ -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>; | ||
|
@@ -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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"); | ||
|
@@ -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, | ||
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_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); | ||
|
@@ -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>>; | ||
|
||
|
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; }); | ||||||||||||||||||||||||||||||||||||
} |
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.
Can this be named has_same_format or something like that? compare sounds like a general == check, when this is only checking the format of the buffer.
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.
Yes, I totally agree, but what is a good name?
Here is my line of though that got me to the new name and source code comment:
"format" has so so many meanings already, to the uninitiated it likely means nothing. Additionally, the
Py_buffer
format
strings may actually not be the same, because of the i/q/l ambiguity I was struggling with here.Trying to start from the top down, what would a good source code comment be?
Asking myself: Is that 100% accurate? What is "is" when it comes to aliases, such as
long double
being effectively an alias fordouble
on some platforms? Orlong
effectively being an alias forint
on some platforms, andlong long
an alias forlong
on others? (I think I actually stumbled over all of these ambiguities at various points in time far back.)Asking myself for completeness: Do we have to worry about endianness? Searching for "endia", "most", "least", "signi" under https://docs.python.org/3/c-api/buffer.html doesn't pull up anything, which I take it means that we don't have to worry. It appears to be assumed that endianness conventions are never mixed.
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 think the precise semantics here are the same as https://en.cppreference.com/w/cpp/types/is_same
// Determines if the butter item type is the same as
T
(see std::is_same)Good point. Yeah, your code is actually technically incorrect as you aren't handling endianness. But it will only provide false negatives. The format strings do specify endianness. This is specified in https://docs.python.org/3/library/struct.html#byte-order-size-and-alignment
Note that if there is no endian specifier, the default is "native", so your code won't return any false positives.
Your current code will return false negatives. "@i" and "=i" for example are equivalent to "int", but your code will return false. "<i" will be equivalent to int on little-endian platforms. ">i" and "!i" on big-endian platforms. Etc, etc.
Not sure if this is worth handling.
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.
Also, why is this a static method and not a member function?
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.
Not sure: last night I saw that
std::is_same<double, long double>::value
with MSVC isfalse
, which is why I had to change my code tosizeof(long double) == sizeof(double)
(b09e75b), because that's what we really want to know.I'm pretty sure no: https://docs.python.org/3/c-api/buffer.html isn't explicit about it (unfortunately), but it's strongly implied that endianness is assumed not to be mixed. Therefore I think going into that in the context of
buffer_info
will not help anything, on the contrary.The situation is very different for serializing/deserializing arrays, then it has to be assumed endianness could be different between the producer and consumer, which could run on totally different platforms.
But the buffer interface is all about in-memory access. Hopefully we won't have one process (one address space) that mixes endianness. (It sounds like an idea from hell.)
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.
Then you can take a simple function pointer, vs a member function pointer. I think that's easier to work with in generic code.
Other than that, it doesn't really matter?
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.
It's rather uncommon to see a static member function that could be a member function since member functions have a better interface for general use. I actually can't think of a single example in the standard library where a static member function is used like this.
It's more of a style thing though, not major.
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.
Ok, I'll try to change it to a member function asap. (I figure it's now or never, i.e. important to get right now.)
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: ef34d29
I agree the
buffer_info
API looks better that way. The only trouble was/is, the syntax for dereferencing member function pointers badly rubs me the wrong way.(buffer.request().*((*equiv_table)[cpp_name]))()
, what a beauty!