-
Notifications
You must be signed in to change notification settings - Fork 68
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
Implement iDynTree base type type_caster
for pybind11 bindings
#931
Conversation
Awesome work @GiulioRomualdi, I'd love to see idyntree pybind11 bindings on par with those autogenerated by SWIG. As we understood in this last period, it's a very effective way to implement interoperability between different projects (modulo some edge case ami-iit/bipedal-locomotion-framework#386). I put this PR in my review list, I also add @francesco-romano as reviewer since he was the original author of the pybind11 version of the bindings. |
1fef829
to
de0c48f
Compare
de0c48f
to
c955175
Compare
The PR is ready to be reviewed. I'm pretty sure that it is possible to improve the performances of the |
The change seems to be nice, however from what I understand it is a breaking change, right?
is not working anymore? It may be worth to clearly document this in the CHANGELOG then. I would also want to make sure that this is ok for @francesco-romano that originally contributed the pybind11 bindings, and I guess is mantaining an internal use of iDynTree/pybind11 code that he may need to migrate. To clarify, this will only be released as iDynTree 5, iDynTree releases <= 4 will keep the non-numpy API. |
@@ -167,7 +161,7 @@ def test_attached_links(self): | |||
self.assertEqual(joint.get_second_attached_link(), 2) | |||
|
|||
def test_rest_transform(self): | |||
position = iDynTree.Position(1, 2, 3) | |||
position = (1, 2, 3) |
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.
Just to understand, why in the test the 3D vectors are sometimes a list and sometimes a tuple?
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.
It's actually the same, position can be a tuple or a list. pybind11 does the magic to convert it in a iDynTree::Position
when required
Yes, this is an important topic that we should discuss. Indeed if a vector of type template <typename Type>
struct type_caster<Type, enable_if_t<is_idyntree_vector_fix_size<Type>::value>> This struct has two methods, For example, given the following c++ class
and the associated pybind11 code PYBIND11_MODULE(example, m) {
py::class_<Foo>(m, "Foo")
.def("set_vector", &Foo::setVector)
.def("get_vector", &Foo::getVector); when you call foo = Foo()
foo.set_vector([1,2,3]) The python list is template <typename Type>
struct type_caster<Type, enable_if_t<is_idyntree_vector_fix_size<Type>::value>> pybind knows how to convert a The same approach is valid for the @traversaro, Let me know if it is more clear know |
Before going into details in the review / testing the changes, I would like to understand better if this is what we really want or not. Focusing on the vector / matrices class only, the change will, in practice, remove those classed from Python. (the alternative approach, to be tested, is to add conversion functions into the vector classes, something like
A second question, related to above: If we are ok with implicit conversion. how do we decide Fixed vs Dynamic version when casting? (I am referring to overloads taking both versions). We do not have control on this anymore. (Maybe we could probably order the cast to try Fixed first?) Third: we would probably lose the ability to resize in-place vectors and matrices. Is this something that might be useful? It is definitely in C++, not sure in Python? |
I think that the evaluation done by @GiulioRomualdi is that we sacrifice explicit typing to the altar of convenience of numpy, but perhaps @GiulioRomualdi can elaborate more. I am not a big user of Python bindings so I would trust users on the choice of this tradeoff.
Not sure on this, what is happening in the version proposed by this PR @GiulioRomualdi ?
I guess also this is something that we sacrifice for using natively numpy. |
Hi, @francesco-romano and @traversaro sorry for replying late.
The idea is exactly this one, we could use directly numpy arrays instead of iDynTree one.
In this case, if you try to call a function that takes as input a
This conversion is exactly the same as what is happening with the eigen vectors in pybind11. An eigen vector becomes a NumPy array. So you cannot call anymore the methods of the eigen vectors because they are actually numpy arrays. Here it is exactly the same |
Sounds good on everything. Just one last question here:
Maybe it does not matter at all but my question is the following. What if you have the following method: void Foo(iDynTree::VectorDynSize& v) {
// Do something with the vector
}
template <int size N>
void Foo(iDynTree::VectorFixedSize<N>& v) {
// Very optimised method!
} Which of the two is taken from Python? |
Regarding this I can write a simple test, by the way I think the easiest way in this case is to create two python functions that have different names |
Co-authored-by: Silvio Traversaro <silvio.traversaro@iit.it>
Hi @francesco-romano, this is the outcome void foo(iDynTree::VectorDynSize&v)
{
std::cout << "vectordynsize"<< std::endl;
}
void foo(iDynTree::VectorFixSize<4>& v)
{
std::cout << "vectorfixedsize"<< std::endl;
}
namespace py = ::pybind11;
PYBIND11_MODULE(pybind, m) {
m.def("foo", py::overload_cast<VectorFixSize<4>&>(::foo))
.def("foo", py::overload_cast<VectorDynSize&>(::foo));
} import idyntree.pybind as idyn
In [2]: idyn.foo([1,1,1])
vectordynsize
In [3]: idyn.foo([1,1,1,1])
vectorfixedsize However, I noticed that the order in the definition of the bindings matters. Indeed if the order is the following one m.def("foo", py::overload_cast<VectorDynSize&>(::foo))
.def("foo", py::overload_cast<VectorFixSize<4>&>(::foo)); I got In [1]: import idyntree.pybind as idyn
In [2]: idyn.foo([1,1,1])
vectordynsize
In [3]: idyn.foo([1,1,1,1])
vectordynsize So in conclusion I would define two different python function such as m.def("foo_vectordynsize", py::overload_cast<VectorDynSize&>(::foo))
.def("foo_vectorfixsize", py::overload_cast<VectorFixSize<4>&>(::foo)); to avoid confusion |
Thanks for the test. I think this should be good enough. |
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.
Please add a test for all the caster functionalities
/** | ||
* Conversion from C++ to Python | ||
*/ | ||
static handle cast(Type src, return_value_policy policy, handle parent) |
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.
return_value_policy policy
this partially answer my previous question, at least for the C++->Python side.
This side is good. We copy the object into Python
52f51c4
to
0b3b349
Compare
729ca28
to
c746633
Compare
This reverts commit c955175.
This comment has been minimized.
This comment has been minimized.
…ybind11/idyntree_core.cpp
25d757b
to
f04e844
Compare
I implemented the tests and I fixed some bugs in the type_caster. |
@GiulioRomualdi sorry, devel was not updated with the latest CI fix. Feel free to merge again now, thanks! |
f4ab83b
to
ec7c8f6
Compare
Sorry for jumping in so late but this PR stayed in my backlog for too long. Thanks @francesco-romano for the thorough review, and @GiulioRomualdi for iterating over it. I do not currently have any downstream code that uses the previous version of the pybind11 bindings, so I fully trust the judgement of @francesco-romano since he is the (only?) user -and original author- of these bindings version, and for sure he's the person with most interest in preserving compatibility / minimizing changes. I went through you comments, and yes, I confirm that in the Python side, when there are multiple binded methods / functions that could cast the same time, their C++ definition order matters as you found out from your tests. What's not 100% clear to me without directly testing the code of this PR is how memory ownership is dealt. The default Eigen / NumPy conversion has two different modalities for input data: pass-by-value that always involves a copy, and pass-by-reference that shares the memory. Instead, for output data, this is the relevant documentation. @GiulioRomualdi, can you please comment about what this PR currently implements? At this first stage, I would not mind on optimizing performances by reducing copies, it can be done in the future if not yet supported, I'd like just to understand what to expect especially from the data returned to Python from the bindings. Also, storage order (column-major vs row-major) is another advanced feature to double check. |
Only the copy is implemented
iDynTree stores the matrices in row-major like numpy so it shouldn't be a problem |
To be precise, this py::array_t<double, py::array::c_style is where you specify the row-major ordering. So you are correctly setting it as row-major. |
I will need to go over the PR again next week. I lost track of what is pending or not (no longer used to GitHub sorry ) |
set(libraryname idyntree-pybind11) | ||
|
||
add_library(${libraryname} INTERFACE) | ||
target_link_libraries(${libraryname} INTERFACE idyntree-core) | ||
|
||
# Specify include directories for both compilation and installation process. | ||
# The $<INSTALL_PREFIX> generator expression is useful to ensure to create | ||
# relocatable configuration files, see https://cmake.org/cmake/help/latest/manual/cmake-packages.7.html | ||
# creating-relocatable-packages | ||
target_include_directories(${libraryname} INTERFACE | ||
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>" | ||
"$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/${CMAKE_INSTALL_INCLUDEDIR}>") | ||
|
||
# Specify installation targets, typology and destination folders. | ||
install(TARGETS ${libraryname} | ||
EXPORT ${PROJECT_NAME}) | ||
|
||
install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/include/iDynTree/pybind11/VectorCasters.h" | ||
DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/iDynTree/pybind11") | ||
|
||
add_library(iDynTree::${libraryname} ALIAS ${libraryname}) | ||
set_property(GLOBAL APPEND PROPERTY ${VARS_PREFIX}_TARGETS ${libraryname}) | ||
|
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.
tl;dr; Why these changes?
If I understood correctly, you want to create a separate library for the caster? Then
- The name is very confusing. It does not explain what the library is for and it is a permutation of the other library name.
INTERFACE
library, no sources? I know this is ok, but I think adding the (single) header file and setting it as public header you can skip theinstall FILES
command and make it more clear- Which should probably be 0): Why? Why do you need the header file outside the build directory? Do you have C++ libraries that depend on this?
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.
INTERFACE library, no sources? I know this is ok, but I think adding the (single) header file and setting it as public header you can skip the install FILES command and make it more clear
In this specific case you can't as there destination path is iDynTree/pybind11
, while the PUBLIC header stuff only support one folder as destination path.
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.
The name is very confusing. It does not explain what the library is for and it is a permutation of the other library name.
We can change it, what do you suggest?
INTERFACE library, no sources? I know this is ok, but I think adding the (single) header file and setting it as public header you can skip the install FILES command and make it more clear
I will need it to import it in a different project that uses idyntree types in the interface. Having an exported interface library simplifies importing it. It is exactly the same as #include <pybind11/eigen.h>
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.
We can change it, what do you suggest?
What about idyntree-pybind11-vector-casters
?
Also, can you remove
set(libraryname idyntree-pybind11)
and use explicitly the new library name?
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.
Can you also:
- move the install part down together with the install of the module?
- What is the purpose of the
ALIAS
?
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.
@GiulioRomualdi what do you think of idyntree-pybind11-vector-casters
@GiulioRomualdi ? It seems a nice name to me.
position = np.array([1., 2., 3.]) | ||
rotation = np.array([[0., 0., 1.], [1., 0., 0.], [0., 1., 0.]]) | ||
transform = iDynTree.Transform(rotation, position) |
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.
This is pending the other discussion
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.
which one?
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.
The one on implicit vs explicit conversion
import numpy as np | ||
|
||
|
||
class TestClassTest(unittest.TestCase): |
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.
[Question]
I am trying to understand what we are testing with this test case.
What we want is to check that Python List/Numpy objects are converted to the corresponding iDynTree classes and vice versa.
Is this test enough for testing this?
Let's take the first test:
def test_vector_fix(self):
obj = TestClass()
vector = [1., 2., 3.]
obj.vector_fix = vector
self.assertEqual(len(obj.vector_fix), 3)
self.assertEqual(list(obj.vector_fix), list(vector))
obj.vector_fix = vector
tests that the conversion into a fix<3> works.
The other two lines are then testing the conversion back into Python.
But, can we add a test that if we have a C++ method operating on the vector it is happy? e.g. summing 1 on all elements.
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.
But, can we add a test that if we have a C++ method operating on the vector it is happy? e.g. summing 1 on all elements.
@GiulioRomualdi any toughts? Also for this I think we can merge in this state and eventually propose new PRs.
@@ -19,7 +43,7 @@ set_target_properties(pybind11_idyntree PROPERTIES | |||
|
|||
# if compile tests execute also python tests | |||
if(IDYNTREE_COMPILE_TESTS) | |||
add_subdirectory(tests) | |||
add_subdirectory(tests idyntree_pybind11_test) |
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.
Do you think there is a way to avoid this and avoid modifying the test Cmake working directory variable?
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.
@GiulioRomualdi any thoughts on this? Note that if it is difficult I prefer to merge the PR in the current form, it is not particularly problematic to have this.
We can close since #1037 implements already one of the key feature designed by this PR |
In the past few days, I discussed with @traversaro about the possibility to install also
pybind11
bindings with thesuperbuild
.This will simplify the usage of
iDynTree
bindings along withbipedal-locomotion-framework
. In the end, we can use the same approach ofmanifpy
ami-iit/bipedal-locomotion-framework#238 (comment)Given that it would be nice to automatically convert
iDynTree
base types (VectorDynSize
,VectorFixSize
,MatrixDynSize
andMatrixFixSize
) toNumPy
objects. Accordingly topybind11
documentation this can be done implement a customtype_caster
.This PR introduces the machinery to automatically convert iDynTree matrices and vectors to numpy.
Furthermore, this PR introduces:
type_caster
rotation
andposition
properties readable and writable in the pybind11 Transform bindingspython
will not exploit the rotation matrix properties (inverse == transpose)Thanks to this PR we can do something like
TODO
cc @Giulero