Skip to content
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

Support direct STL container wrapping in the Pybind11 wrapper #1076

Closed
varunagrawal opened this issue Jan 30, 2022 · 2 comments · Fixed by #1546
Closed

Support direct STL container wrapping in the Pybind11 wrapper #1076

varunagrawal opened this issue Jan 30, 2022 · 2 comments · Fixed by #1546
Assignees
Labels
feature New proposed feature

Comments

@varunagrawal
Copy link
Collaborator

Feature

By including pybind11/stl.h, we can have Pybind11 directly translate STL container elements (even those that are typedef'd) rather than go the roundabout way of defining a custom opaque type that may collide with another type.

For example, recently #1069 added a Rot3Vector to the wrapped code and this shouldn't be necessary. We should just be able to say std::vector<gtsam::Rot3> and use a regular list on the python side. This makes the wrapper more powerful and more intuitive.

The downside is that this will considerably change the Python API (using python based data types rather than our custom types) but it is the more beneficial approach in the long term.

Motivation

The hassle we had with #1069 is some pretty good motivation.

Pitch

I took the day off yesterday and worked a bit on this to see how much effort it was, and I made surprisingly great progress. I can make a PR as early as today.

@varunagrawal varunagrawal added the feature New proposed feature label Jan 30, 2022
@dellaert
Copy link
Member

If it is very much API breaking I want to hold off until next major version (5.x). The book development can't tolerate this amount of disruption at this time.. Also, IROS....

@varunagrawal
Copy link
Collaborator Author

Yup thought as much. I'll still open a draft PR on my fork so we're not starting from scratch in the future and not wasting CI cycles.

@varunagrawal varunagrawal linked a pull request Jun 16, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New proposed feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants