Generic containers/iterators for shape/strides #788

jagerman · 2017-04-07T20:22:43Z

This adds support for constructing buffer_info and arrays 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 a ssize_t and size_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.

jagerman · 2017-04-07T20:28:42Z

@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).

dean0x7d · 2017-04-07T20:40:04Z

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.

jagerman · 2017-04-07T20:43:25Z

That's a good idea. The same thing could work for the iterator versions.

SylvainCorlay · 2017-04-07T20:47:38Z

@jagerman @dean0x7d we now use the Numpy API directly and only inherit from py::object in our pybind11 bindings.

jagerman · 2017-04-07T22:57:41Z

Pushed update with @dean0x7d 's adaptor suggestion.

aldanor · 2017-04-07T23:04:31Z

include/pybind11/common.h

+// 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


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.

Good point. There is something a bit icky about moving a struct's member.

aldanor · 2017-04-07T23:05:35Z

include/pybind11/numpy.h

@@ -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(),


Maybe just provide a const ref .data() on the container?

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.

Actually, that won't work; clang considers it ambiguous.

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)

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?

dean0x7d · 2017-04-12T00:12:25Z

include/pybind11/buffer_info.h

+    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)


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});

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.

jagerman · 2017-04-12T15:22:14Z

Any other comments on this before I merge it? (This is needed PR #782 and would be nice for #795 as well).

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).

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).

aldanor · 2017-04-13T07:00:56Z

Looks good to me.

wjakob · 2017-04-28T12:53:50Z

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,

size_t n = ..., m = ...;
py::array_t<float> array({n, m});

is now ambiguous. Same issue with this code:

size_t n = ..., m = ...;
py::array_t<float> array({(ssize_t) n, (ssize_t) m});

I had to change all occurrences of this pattern to

py::array_t<float> array(std::vector<ssize_t>{(ssize_t) n, (ssize_t) m});

for the compiler to accept it. I would be in favor of continuing to allow initializer lists for convenience -- what do you think?

jagerman · 2017-04-28T14:19:11Z

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 any_container: so {1, 2} could either be a implicit construction of an array_t({shape={1}, strides={2} }), or an implicit construction of an any_container with the initializer list. You can fix it with py::array_t<float> array({{n, m}}), but that's not a nice solution.

I think taking out the arithmetic constructor makes the most sense here, so any_container needs a container (or initializer_list), but won't accept a single scalar value. That was convenient, but not at the cost of introducing ambiguity.

jagerman mentioned this pull request Apr 7, 2017

Python buffer objects can have negative strides. #782

Merged

aldanor reviewed Apr 7, 2017

View reviewed changes

jagerman force-pushed the iterator-shape-strides branch 5 times, most recently from 05d34a5 to ad75327 Compare April 10, 2017 22:30

dean0x7d reviewed Apr 12, 2017

View reviewed changes

jagerman mentioned this pull request Apr 12, 2017

array: implement array resize #795

Merged

jagerman added 2 commits April 12, 2017 18:03

jagerman force-pushed the iterator-shape-strides branch from feef3f3 to 72cfdda Compare April 12, 2017 22:08

jagerman merged commit 201796d into pybind:master Apr 13, 2017

jagerman mentioned this pull request Apr 28, 2017

Fix ambiguous initialize_list arguments #822

Merged

dean0x7d modified the milestone: v2.2 Aug 13, 2017

rwgk mentioned this pull request Feb 9, 2023

FWD pybind11 google/pybind11clif#788

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic containers/iterators for shape/strides #788

Generic containers/iterators for shape/strides #788

jagerman commented Apr 7, 2017

jagerman commented Apr 7, 2017

dean0x7d commented Apr 7, 2017

jagerman commented Apr 7, 2017

SylvainCorlay commented Apr 7, 2017

jagerman commented Apr 7, 2017

aldanor Apr 7, 2017 •

edited

Loading

jagerman Apr 7, 2017

aldanor Apr 7, 2017

jagerman Apr 7, 2017

jagerman Apr 7, 2017

aldanor Apr 7, 2017

aldanor Apr 8, 2017

dean0x7d Apr 12, 2017

jagerman Apr 12, 2017

jagerman commented Apr 12, 2017

aldanor commented Apr 13, 2017

wjakob commented Apr 28, 2017

jagerman commented Apr 28, 2017

Generic containers/iterators for shape/strides #788

Generic containers/iterators for shape/strides #788

Conversation

jagerman commented Apr 7, 2017

jagerman commented Apr 7, 2017

dean0x7d commented Apr 7, 2017

jagerman commented Apr 7, 2017

SylvainCorlay commented Apr 7, 2017

jagerman commented Apr 7, 2017

aldanor Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman commented Apr 12, 2017

aldanor commented Apr 13, 2017

wjakob commented Apr 28, 2017

jagerman commented Apr 28, 2017

aldanor Apr 7, 2017 •

edited

Loading