Add the buffer interface for wrapped STL vectors

[patstew]

Allows use of vectors as python buffers, so for example they can be adopted without a copy by numpy.asarray
Allows faster conversion of numeric buffers to vectors with memcpy instead of individually casting the elements

[aldanor]

FYI: note that you can already wrap vectors as numpy arrays without copying, kind of like so:

struct Data {
    std::vector<int> vec;
};

py::class_<Data>(m, "Data")
    .def("vec", [](Data& self) -> py::array {  // no copying
        return {self.vec.data(), self.vec.size(), py::cast(self);
    });

Untested, but something like this should work.

[patstew]

That's cool, I didn't know that. But going that way would require me to write a load of little wrapping lambdas for my functions, or change my C++ API to return those proxy objects. This way seems easier to me, and the buffer interface has more general use without necessarily using numpy.

[wjakob]

Neat!

@aldanor: the advantage of this version is that the original instance exposes all the necessary interfaces. (i.e. no need to call an extra function like .vec()).

The restriction to arithmetic-only types if of course the main disadvantage - @aldanor's NumPy interface can handle much fancier structured data types.

I wonder if there is a way to expose a NumPy interface (analogous to the buffer interface) that works for more complex structured types, and without calling an extra conversion function?

[patstew]

I guess it would be possible to write some template machinery to let you explicitly declare members of a struct, or use a std::tuple, and match the types to the python format. But I'd guess that you'd run into problems with C++ and python disagreeing about alignment in those cases? I guess it'd be OK for a Point { float x, float y } sort of thing, but once you start mixing types is the memory layout guaranteed?

[aldanor]

@patstew That's exactly what "record array" part of <pybind11/numpy.h> does :) E.g. figures out the exact offsets, alignment, supports nested structs, etc, but you have to call a macro and pass it the field names. A fairly good demo of supported types is in the tests.

Why the "is_arithmetic" restriction? I don't why this would work with structured dtypes. PYBIND11_NUMPY_DTYPE registeres both npy_format_descriptor<> and format_descriptor<>, so it should work in this case just as well?

Re: "not bool", this bit looks a tad flaky; I guess what you really want to check is that the container type is not a proxy and underlying memory is contiguous. Simply checking that .data() returns a pointer is sufficient for STL containers, I think (vector<bool> doesn't have it in the specialization).

[patstew]

There we go, it now goes to and from structured arrays too. I've stopped pybind11 from declaring aligned types as unaligned in the format string and added a function to compare format strings, so that I can tell when a numpy structured dtype and a c++ one are the same, even when the format strings are not literally the same. numpy tends to declare padding as 'xxx' rather than '3x' and doesn't add trailing padding to the format string. At least for me on windows, numpy and c++ don't agree on whether int32 is int or long so I allow 'i' == 'l' when appropriate. I'm also ignoring the field names.

[aldanor]

Ok, so it looks like this is now 2 (3?) separate issues, and as such it may even make sense to break it down into separate PRs.

1. Recording alignment of fields for structured dtypes. Makes sense to have -- however it would be also nice to add a test that actually fails without this (i.e. at the current master branch) at least on one platform. (We also ignore byte ordering at the moment btw)

2. The actual vector/buffer interface

Re: compare_format_descriptor(), congrats, you've started partially reimplementing functionality from numpy.core._internal :) While your code is likely to be correct (not clear; needs extensive tests if this is to be merged in), tbh I wouldn't be a huge fan of maintaining it. Buffer format strings are horrible, and there's always a myriad of weird corner cases.

One alternative option would be to rely on numpy for comparing structured types (you have to register them through numpy anyway, so this makes sense). You'll still need special-casing even for arithmetic types, but now that would be a lot simpler. Another option is to use numpy dtypes for comparing all underlying value types; this imposes reliance on numpy but would be a bit cleaner.

There's already a py::dtype(const buffer_info&) ctor, and there's py::dtype::of<T>() for all registered types. Dtypes can then be compared for equivalence (not equality), which should handle the corner cases like sizeof(int) == sizeof(long) on Windows platforms (will still need tests).

[aldanor]

@wjakob @patstew

@aldanor: the advantage of this version is that the original instance exposes all the necessary interfaces. (i.e. no need to call an extra function like .vec()).

Well, there's NumPy array interface which is very simple to implement and doesn't require dancing around Python buffer protocol and its format strings. Then the original instance would also "expose all the necessary interfaces" as you could just np.asarray(vec), or even just pass it in places where numpy array is expected.

[patstew]

Ok, I've cut out some of the extra stuff for this PR. It basically means that the fast init path won't work as often, but exposing vectors works fine. I've added some non-numpy tests too.
My original reason for adding this was so that passing around big vector<char>s (from file/network/etc) wasn't doing quite so much unnecessary work. The numpy aspect was a nice to have, and handy for testing across py2/py3. I'd prefer not to tie the whole thing to numpy.
Perhaps we could have a simple compare_format_descriptor() (perhaps just string ==) and overload it if py::dtype::of<T> exists?
I've split the unaliged buffer bit into a separate PR, and can do a new PR for the compare_format_descriptor() later with some guidance on what form is preferable.

[aldanor]

@patstew So, it looks like the cases are:

1. T is a non-structured type. In this case you can compare format strings, and even handle some platform-dependent weirdness manually (like i and l on Windows). Maybe it'd make sense to have a normalize_scalar_format() function that would handle all that instead of a comparison function? With a bit of effort, you could probably even make checks like sizeof(int) == sizeof(long) compile-time.

2. T is a structured type (basically, format string is not a single character, or it contains '{'?). Currently, you can only register those via PYBIND11_NUMPY_DTYPE, so we know that NumPy is present, and we can compare dtypes for equivalence, namely py::dtype::of<T>() and dtype constructed from buffer info object.

Note that you could still use NumPy dtype comparison in (1), but if comparing format strings for simple scalars is sufficient then it's fine.

[aldanor]

Here's another thought, maybe there could be a separate type for format strings, e.g. buffer_format, implicitly convertible from/to std::string so as not to break existing code, and implementing comparison op following the logic above. This way it could be more reusable, since it's not stl-vector-specific inherently.

[patstew]

Using the numpy comparison function seems to fail when there's trailing padding required, because when you create a dtype from the buffer string numpy returns there is no trailing padding, so EquivTypes returns false. I suspect it's to do with this numpy/numpy#7798
Code with failing test here: https://github.com/patstew/pybind11/tree/compare_buffer_numpy

I still think this PR is useful on its own. Are there any issues with it? Making it accept more struct types can be added as a new PR.

[aldanor]

@patstew Yes, you can notice my comments in numpy/numpy#7797 and numpy/numpy#7798.

This is why we have these bits of code in pybind11:

• https://github.com/pybind/pybind11/blob/master/include/pybind11/numpy.h#L771
• https://github.com/pybind/pybind11/blob/master/include/pybind11/numpy.h#L272

[aldanor]

(as in, if the padding doesn't get stripped properly by py::dtype, it's a bug we have to fix regardless; I'll try to take a look at that branch)

[wjakob]

Not sure I understand this? Are we treating GCC specially here?

[patstew]

As far as I can tell just doing SFINAE with decltype(std::declval<Vector>().data()) does what I expect on MSVC, but it gets enabled for vector<bool> on GCC (which shouldn't have a data member). !is_same_v<decltype(...), void> works for both, which suggests to me that GCC sees void data(), though I may be misunderstanding.

[wjakob]

If the goal is just to detect the bool special case, wouldn't it be more transparent to SFINAE on that?

[patstew]

I did that originally, aldanor said he'd prefer checking for data().

[wjakob]

cc @aldanor

[aldanor]

Yes, GCC has void std::vector<bool>::data(). If vector's data() returns a pointer type, it's not a proxy, I guess this was my initial point. But if the consensus is to just check for bool so the sfinae is simpler so be it....

[jagerman]

Instead of checking that it isn't void, why don't you check that it is a Vector::value_type *? (Ultimately that seems to align better with what actually matters in the code)

[jagerman]

There's nothing, in theory, to prevent this from working with some std::vector-like custom class (just like bind_vector), so I think the check is worthwhile, but it should really just check for what we want instead of checking for not being what the current stdlibc++ implementation does.

[wjakob]

So what happens if stl_bind.h is included but not numpy.h?

[patstew]

It works for the ordinary arithmetic types declared in common.h, or fails to compile because the default implementation of py::format_descriptor is empty. numpy.h provides a specialisation for all POD types, which may throw at runtime if the type isn't declared properly.
The py::format_descriptor<typename Vector::value_type>::format() SFINAE bit means it never hits the 'fail to compile' case.

[wjakob]

Ok -- this sounds good to me then.

[wjakob]

Note: this PR is currently conflicted and needs to be rebased on top of master.

[patstew]

I've fixed the issue with padding in that branch (https://github.com/patstew/pybind11/tree/compare_buffer_numpy) now. I'll submit it as a new PR once this has gone in, unless you'd like me to add it to this PR. I suspect it warrants its own discussion though.

[aldanor]

@patstew Yes, the strip_padding fix/hack looks correct to me; I actually fixed it almost exactly the same way locally while looking at why your branch fails.

Would you mind opening a separate PR just for that first? (+ a test in test_numpy_dtypes that would be broken on the current master but would get fixed with the strip_padding fix)

[patstew]

Appveyor seems to have taken an hour and not actually started building at all...

[jagerman]

No, it's more of a you-'re-on-your-own as far as lifetime management, essentially the same thing you get with rvp::reference.

[patstew]

This was broken by the addition of py::buffer_protocol(), I've fixed that and a merge conflict. I'm not sure why AppVeyor is refusing to build it?

[dean0x7d]

The CI hooks may not have triggered correctly. Try pushing a new commit and see if it recovers.

[patstew]

@wjakob @aldanor @jagerman Are there any outstanding issues with this?

[jagerman]

@patstew: the PR currently disables (comments out) most of the travis-ci builds, which obviously need to be undone.

[jagerman]

I don't think this strides[0] == sizeof(T) requirement is desirable—it seems only to be a requirement of the vector constructor you are using, not of the type itself. It should be easy enough to support arbitrary strides, which let slices (e.g. single rows/columns of numpy matrices) work as vector buffer inputs. Something like this (untested) as a replacement for the Vector constructor ought to work:

new (&vec) Vector();
vec.reserve(info.shape[0]);
for (void *ptr = info.ptr, void *end = info.ptr + info.strides[0] * info.shape[0]; ptr != end; ptr += info.strides[0])
    vec.push_back(*static_cast<T *>(ptr));

(A small test to see that this really does work would be good as well).

[patstew]

Added with test

[jagerman]

Nice 👍

[wjakob]

I took a brief look at the latest iteration of this PR. I realize that it has been in the works for quite a while. Right now I'm still not 100% excited about it, for these reasons:

1. It's a quite heavy addition to add the buffer interface to every std::vector binding. I'm concerned about codebases which use many different kinds of vectors and potentially don't even need this feature. It would be good if this is opt-in.

2. Some parts of this patch could be implemented more elegantly building on top of the existing array_t<>.I'm talking about stuff like

for (char *p = static_cast<char*>(info.ptr), *end = static_cast<char*>(info.ptr) + info.shape[0] * info.strides[0]; p < end; p += info.strides[0])
            vec.push_back(*reinterpret_cast<T*>(p));

In practice, I suppose that non-trivial types will require NumPy support in any case.

Thoughts?

[patstew]

I've made it conditional on the existing py::buffer_protocol() option.
I've only just added that particular monstrosity to deal with strided arrays as @jagerman requested. I'd prefer not to make this wholly tied to numpy or array_t, my original motivation was that passing vectors of several MBs for IO was taking a noticeable amount of time going through all the cast functions. The more advanced numpy structs and strides stuff is there because you guys requested it :).

[jagerman]

That monstrosity could probably be made much more readable if the *end = static_cast<char*>(info.ptr) + info.shape[0] * info.strides[0] was before the for loop instead of as a double initializer. (I take the blame; that was copied directly from my suggestion).

[patstew]

I've got rid of the reinterpret_cast too, by assuming (and checking) that strides[0] is a multiple of itemsize (Python docs say it should be) and itemsize == sizeof(T) (which it should if the format matched).

[dean0x7d]

This seems like a very useful feature and it's been cooking for a while. Are there any blockers left?

[wjakob]

No, I think it looks great now! Thank you @patstew!

[wjakob]

(I've added you to the acknowledgements in README.md)