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

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

Open
traversaro opened this issue Sep 14, 2020 · 5 comments

Comments

@traversaro
Copy link
Member

See discussion in #736 (comment) .

@diegoferigo
Copy link
Member

diegoferigo commented Sep 14, 2020

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

@traversaro
Copy link
Member Author

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.

@diegoferigo
Copy link
Member

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.

@francesco-romano
Copy link
Collaborator

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

@diegoferigo
Copy link
Member

diegoferigo commented Sep 16, 2020

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.

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

No branches or pull requests

3 participants