-
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
Generic containers/iterators for shape/strides #788
Conversation
@SylvainCorlay / @JohanMabille - I have a vague recollection that it was one (or both) of you who wanted something like this. (Apologies if I'm wrong about that--I couldn't find the discussion). |
I wonder if this could be simplified by shifting most of the logic from the constructors into a kind of vector adaptor class. E.g., instead of: template <typename Shape, typename Strides,
typename = decltype(std::begin(std::declval<const Shape &>())),
typename = decltype(std::begin(std::declval<const Strides &>()))>
array(const Shape &shape, const Strides &strides); use: array(const Adaptor& shape, const Adaptor& strides); where: class Adaptor {
public:
template <typename Container, typename /*= ... SFINAE*/>
Adaptor(const Container& c);
private:
std::vector<T> data; // or whatever
}; So instead of templating every constructor, the adaptor takes care of everything. |
That's a good idea. The same thing could work for the iterator versions. |
Pushed update with @dean0x7d 's adaptor suggestion. |
include/pybind11/common.h
Outdated
// container (or C-style array) supporting std::begin/std::end. | ||
template <typename T> | ||
struct any_container { | ||
std::vector<T> v; // Intended to be std::move'd |
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.
Nitpicking: if you don't want to make v
public, you could provide
operator std::vector<T>&&() && { return std::move(v); }
and then move the vector directly out of the container.
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.
Good point. There is something a bit icky about moving a struct's member.
include/pybind11/numpy.h
Outdated
@@ -478,7 +480,7 @@ class array : public buffer { | |||
} | |||
|
|||
auto tmp = reinterpret_steal<object>(api.PyArray_NewFromDescr_( | |||
api.PyArray_Type_, descr.release().ptr(), (int) ndim, shape.data(), strides.data(), | |||
api.PyArray_Type_, descr.release().ptr(), (int) ndim, shape.v.data(), strides.v.data(), |
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.
Maybe just provide a const ref .data()
on the container?
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 added an lvalue reference conversion operator (in addition to the rvalue operator), so I can just grab a vector reference before that and call .data()
on it.
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.
Actually, that won't work; clang considers it ambiguous.
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.
Weird, it's not obvious how it could be ambiguous.
So maybe just .data()
then? Would we need anything else from it? (data ptr / size / move out into a vec)
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.
Btw you only use the lvalue (const) ref in passing it to default_strides()
though? So maybe if you remove the mutable &
overload and leave just &&
and const &
there would be no ambiguity?
05d34a5
to
ad75327
Compare
include/pybind11/buffer_info.h
Outdated
template <typename ShapeIt, typename StridesIt, | ||
typename = detail::enable_if_t<detail::is_input_iterator<ShapeIt>::value && detail::is_input_iterator<StridesIt>::value>> | ||
buffer_info(void *ptr, size_t itemsize, const std::string &format, size_t ndim, | ||
ShapeIt shape_first, ShapeIt shape_last, StridesIt strides_first, StridesIt strides_last) |
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.
Isn't this already covered by the any_container
constructor?
// ShapeIt, StridesIt
buffer_info(..., shape_first, shape_last, strides_first, strides_last);
// any_container
buffer_info(..., {shape_first, shape_last}, {strides_first, strides_last});
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.
Yeah, you're right. I was intending to provide a more stl-like interface, but cutting down the number of constructors seems like a better idea.
Upcoming changes to buffer_info make it need some things declared in common.h; it also feels a bit misplaced in common.h (which is arguably too large already), so move it out. (Separating this and the subsequent changes into separate commits to make the changes easier to distinguish from the move.)
This adds support for constructing `buffer_info` and `array`s using arbitrary containers or iterator pairs instead of requiring a vector. This is primarily needed by PR pybind#782 (which makes strides signed to properly support negative strides, and will likely also make shape and itemsize to avoid mixed integer issues), but also needs to preserve backwards compatibility with 2.1 and earlier which accepts the strides parameter as a vector of size_t's. Rather than adding nearly duplicate constructors for each stride-taking constructor, it seems nicer to simply allow any type of container (or iterator pairs). This works by replacing the existing vector arguments with a new `detail::any_container` class that handles implicit conversion of arbitrary containers into a vector of the desired type. It can also be explicitly instantiated with a pair of iterators (e.g. by passing {begin, end} instead of the container).
feef3f3
to
72cfdda
Compare
This further reduces the constructors required in buffer_info/numpy by removing the need for the constructors that take a single size_t and just forward it on via an initializer_list to the container-accepting constructor. Unfortunately, in `array` one of the constructors runs into an ambiguity problem with the deprecated `array(handle, bool)` constructor (because both the bool constructor and the any_container constructor involve an implicit conversion, so neither has precedence), so a forwarding constructor is kept there (until the deprecated constructor is eventually removed).
Looks good to me. |
I just noticed a regression in some of my code regarding this patch: initializer lists seem to no longer work, which is a bummer. In particular,
is now ambiguous. Same issue with this code:
I had to change all occurrences of this pattern to
for the compiler to accept it. I would be in favor of continuing to allow initializer lists for convenience -- what do you think? |
Definitely a bug (and only occurs with exactly 2 arguments in the initializer list; with 1 or 3 it's fine). It's caused by the arithmetic constructor in I think taking out the arithmetic constructor makes the most sense here, so |
This adds support for constructing
buffer_info
andarray
s using arbitrary containers or iterators instead of requiring a vector.On its own, this adds quite a few templated constructors which isn't all that desirable, but it's needed by PR #782 to preserve backwards compatibility for previous pybind11 versions that accept a
std::vector<size_t>
for strides. Given a choice between duplicating all the stride-taking constructors (accepting both assize_t
andsize_t
vector) and making the whole interface more general, I think this is a better approach.I also seem to recall some discussion a few months ago about wanting something like this, but I can't find the issue/PR where it was mentioned.