idyntree icon indicating copy to clipboard operation
idyntree copied to clipboard

Evalute to deprecate and remove non-span methods in iDynTree::KinDynComputations

Open traversaro opened this issue 3 years ago • 5 comments

See discussion in https://github.com/robotology/idyntree/pull/736#discussion_r487378372 .

traversaro avatar Sep 14 '20 08:09 traversaro

I am not sure if this deprecation will play well when iDynTree is used from other languages as Python. I fear that the Python code would become very complex due to repeated manual conversions e.g. from VectorDynSize to a iDynTree::Span object.

On the long run, perhaps it would be nice to have an automatic conversion between these types, and I mean allowing to pass a VectorDynSize parameters that gets converted automatically to a iDynTree::Span. Though, I have the feeling that SWIG does not like it. In this case there's no inheritance involved (fully supported by SWIG), and I suspect that automatic type conversion from the target language is not supported.

Perhaps we could solve by adding to all types that could be spann-able a {to/as}Span() method? cc @GiulioRomualdi

diegoferigo avatar Sep 14 '20 09:09 diegoferigo

Perhaps we could solve by adding to all types that could be spann-able a {to/as}Span() method? cc @GiulioRomualdi

Personally if the alternative is the need to add .asSpan to all iDynTree native Matrix/Vector, I would not be to afraid to just keep both the span and the old methods around.

traversaro avatar Sep 14 '20 09:09 traversaro

I would not be to afraid to just keep both the span and the old methods around.

I'm not so against it since after all also in C++ the APIs would require using the iDynTree::to_span() function, if I understood well. Maybe adding the toSpan() could be done in the .i as done for the toNumpy() / fromPython() to only those classes that need it.

It can be used for both input and output arguments, keeping the usage of the bindings very similar, excluding the need to pre-allocate the output objects. Note that the recent NumPy support would anyway require a copy for output arguments.

Digression: what's nice about spans is that they could be easier to map from NumPy types (which are passed as pointer + size... that is a span!). Using spans could be the key in the future to provide native NumPy support for input arguments. Though, mixing NumPy and iDynTree types could be confusing to the end user. Furthermore, output arguments would be more delicate to handle than input arguments, I don't want to add too many details abot since this issue is not strictly Python related.

diegoferigo avatar Sep 14 '20 09:09 diegoferigo

I don't remember SWIG but pybind11 allows cast functions to convert types. I am sure SWIG has the same. One advantage is that you will not copy data between Python and C++. Note: this has to be done with Numpy arrays as native Python lists will be immutable across language

francesco-romano avatar Sep 15 '20 14:09 francesco-romano

Digression: what's nice about spans is that they could be easier to map from NumPy types (which are passed as pointer + size... that is a span!). Using spans could be the key in the future to provide native NumPy support for input arguments. Though, mixing NumPy and iDynTree types could be confusing to the end user. Furthermore, output arguments would be more delicate to handle than input arguments, I don't want to add too many details abot since this issue is not strictly Python related.

I don't remember SWIG but pybind11 allows cast functions to convert types. I am sure SWIG has the same. One advantage is that you will not copy data between Python and C++. Note: this has to be done with Numpy arrays as native Python lists will be immutable across language

I'm not sure to follow with cast functions, can you provide a small example of what you mean?

numpy.i has 4 ways to return data to Python from C++: argout arrays, in-place arrays, argout view arrays, and argout viewm arrays. I think only in-place arrays would allow the use case we're discussing.

The existing NumPy integration of #726 is however different, it just allows to convert iDynTree <-> NumPy, but then all the wrapped methods take iDynTree types. I used the safer argout viewm arrays that alway involves a dynamic allocation and a copy in the SWIG glue code.

What we're talking now, instead, I think is passing directly pre-allocated NumPy arrays to the wrapped iDynTree methods. This is more difficult. If we want to accomplish this, we could maybe exploit in-place arrays and create and then apply a custom typemap as follows:

%apply (double* INPLACE_ARRAY1, int DIM1) {(iDynTree::Span<double> out)};

that automatically adds the necessary glue code to convert the NumPy buffer to a span and then passes it to iDynTree. I never wrote a typemap and I have no idea if this could work. If it works though, in just one-line + a simple typemap we can implement this usage. Let's keep this approach in mind.

diegoferigo avatar Sep 16 '20 08:09 diegoferigo