Skip to content

std::vector from py::iterable #1773

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

Open
davidhewitt opened this issue Apr 26, 2019 · 3 comments · May be fixed by #2950
Open

std::vector from py::iterable #1773

davidhewitt opened this issue Apr 26, 2019 · 3 comments · May be fixed by #2950
Assignees

Comments

@davidhewitt
Copy link
Contributor

At the moment list_caster in stl.h explicitly checks whether the input argument is a sequence, but I would like to argue that that's slightly too restrictive and iterable should be accepted.

The only potential downside I can think of is that generators should definitely not be accepted as a failed conversion of an element halfway through the generator would leave things in a terrible state, but a well-placed PyGen_Check could prevent that.

@lonestarr
Copy link

I had the same issue (I think): if __getitem__ is bound in a class, you cannot use this class as an argument if the same argument can be a std::vector too.

C++

class MyClass
{
public:
    template<class T> void do_something(const T& t)
    {
        std::cout << typeid(t).name() << std::endl;
    }
};

class Iterable
{
public:
};

namespace py = pybind11;

PYBIND11_MODULE(test_module, m)
{
    m.doc() = "test module to see the bug";

    py::class_<Iterable>(m, "Iterable")
        .def(py::init<>())
        .def("__getitem__", [](Iterable& self, int idx) -> int { return 0; });

    py::class_<MyClass>(m, "MyClass")
        .def(py::init<>())
        .def("do_something", [](MyClass& self, const int& t) { self.do_something<int>(t); })
        .def("do_something", [](MyClass& self, const std::vector<int>& t) { self.do_something<std::vector<int>>(t); })
        .def("do_something", [](MyClass& self, Iterable& t) { self.do_something<Iterable>(t); });
}

Python

from test_module import MyClass, Iterable

a = MyClass()
it = Iterable()

a.do_something(5)
a.do_something([5, 10])
a.do_something(it)

the last line raises ValueError: vector<T> too long
It works if I remove the binding code with std::vector

@EricCousineau-TRI EricCousineau-TRI self-assigned this Apr 13, 2021
@EricCousineau-TRI
Copy link
Collaborator

+1 on the front of any stl container type (e.g. std::set<>, std::unordered_set<>, etc.).
I think it may be a bit hairy to change existing stricter behavior, though.

Maybe better is just being explicit. At present, you could do:

.def("do_something", [](py::iterable items) { auto temp = items.cast<std::vector<T>>; ...; })

but that does defeat element-wise type-checking.
Perhaps there's a way to reflect how pybind11/numpy.h does this, e.g. py::array is the type-erased version, and py::array_t<> admits compile-time constraints.

Then something like py::iterable_t<Item> could be used.

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Apr 13, 2021

My initial attempt to encode this in dowstream lib is here:
(WARNING: Not working yet) Works now, but still needs some refinement.
RobotLocomotion/drake#14898
https://github.com/RobotLocomotion/drake/blob/c172b585316e5dbd0542e141108ee928795889d3/bindings/pydrake/geometry_py.cc#L36-L106

wangxf123456 pushed a commit to google/clif that referenced this issue Aug 16, 2022
PyCLIF supports this: https://source.corp.google.com/piper///depot/google3/third_party/clif/python/stltypes.h;l=894;bpv=1;rcl=446801735.

The feature request is tracked at pybind/pybind11#1773.

Also change type caster of `PyObject` so that it does not own the Python object.  Otherwise I see some weird test failures related to gc: http://sponge2/c868d236-550c-4fe5-8d41-0d9ebb8aaf38 (Failed).

PiperOrigin-RevId: 462463815
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants