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

[WIP] [Python Bindings] Changed Vector/Matrix map to Python objects #484

Conversation

francesco-romano
Copy link
Collaborator

tl;dr; instead of using Python function to generate and parse string to convert between types, we now use native C code.

This might have an impact on current Python code. For sure, it will require some modifications:

  • removed the helper functions as they are no longer needed
  • renamed fromList to fromPython (as it now accepts both lists and Numpy arrays).

Tests are missing. I do not have the bandwidth to implement them.
Let me know what do you think - if something is missing.

Of course, waiting for travis to catch all the bugs 😄

%{
#include <iDynTree/Core/VectorDynSize.h>
#include <iDynTree/Core/VectorFixSize.h>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@traversaro not sure I need to include all these files. For example, I forgot to include Wrench.h, etc. and it seems to work.

@traversaro
Copy link
Member

cc @diegoferigo @anqingd and all the Python users around.

@traversaro
Copy link
Member

There are a few users of the iDynTree's Python API . I would like to implement proper deprecation of the old APIs before removing them.

@francesco-romano
Copy link
Collaborator Author

francesco-romano commented Sep 24, 2018

Should be enough to add a fromList that maps to fromPython I think. It can be done in the old init_helpers. While the old init_numpy_helpers can become a simple noop (pass)

@francesco-romano francesco-romano changed the title [Python Bindings] Changed Vector/Matrix map to Python objects [WIP] [Python Bindings] Changed Vector/Matrix map to Python objects Sep 24, 2018
@francesco-romano
Copy link
Collaborator Author

While working on some tests, I realised the PR is not ready as fromPython should be a sort of factory method, while now it is a simple method acting on an already-built object.

Removed the python functions `init_helpers` and `init_numpy_helpers` that were used
to create Vector/Matrix objects from Python lists, and for transforming Vector/Matrix into
NumPy objects.

`toNumPy()` has kept the same name, so no change in code should be needed. It has been
implemented in C++ instead of using Python.
`fromList` has been removed. Now there is a new function `fromPython` (written in C++) that
accepts either a NumPy array (of the proper dimension) or a Python List (again, of the
proper dimension).
Both functions have been implemented for [Matrix|Vector][Fix|Dyx]Size and for
Wrench, SpatialAcc and Twist.
Warnings are raised when the deprecated code is called.
Added also tests for the deprecated functions.
@francesco-romano francesco-romano force-pushed the fix/python_vector_bindings branch from b25bc9f to fa1bc55 Compare September 25, 2018 13:56
@francesco-romano
Copy link
Collaborator Author

Can somebody run the python tests?
I tried with:

  • Python 2.7.10 (default on my Mac 10.13.6) and I get a SegFault
  • Python 2.7.11 (on travis) same error as before
  • Python 2.7.13 and it works

@traversaro
Copy link
Member

Handy recap of Python version on different systems: https://repology.org/metapackage/python2/versions .

@francesco-romano francesco-romano changed the title [WIP] [Python Bindings] Changed Vector/Matrix map to Python objects [Python Bindings] Changed Vector/Matrix map to Python objects Sep 26, 2018
@traversaro traversaro changed the title [Python Bindings] Changed Vector/Matrix map to Python objects [WIP] [Python Bindings] Changed Vector/Matrix map to Python objects Feb 15, 2019
@diegoferigo
Copy link
Member

I tried a rebased version of this branch on Ubuntu 18.04 with Python 3.6 and I've got the following failures:

The following tests FAILED:
         51 - PythonHelperTests (SEGFAULT)
         52 - PythonDeprecatedTests (SEGFAULT)

After a quick execution in the interpreter of the additional binded methods I discovered that fromNumpy() and toNumpy() work well. The segfault seems due the methods that use regular python lists.

cc @francesco-romano @traversaro

@francesco-romano
Copy link
Collaborator Author

Happy if you are able to rebase this and merge it. It has remained here for far too long.

This problem does not look new to me. I am sure I read something similar recently somewhere. I think there has been some changes to Python lists in some 3.x version. Hopefully I will find again the discussion

@traversaro
Copy link
Member

@diegoferigo Did you worked on this recently as part of the development of Python-based controllers?

@diegoferigo
Copy link
Member

@traversaro I am using a rebased version of this branch since some while without any problem. Though, segfaults without any errors occur if they're not used correctly, and debugging these problems can be challenging in a Python project. I think this PR is a great start, but it needs some polishing to become stable enough.

I keep postponing this activity due to other priorities, but I have to do it before releasing the first stable release of gym-ignition in any case.

@traversaro
Copy link
Member

As this PR is currently stale, I will close it, but feel free to re-open it if someone wants to work on this, thanks!

@traversaro traversaro closed this Apr 16, 2020
@diegoferigo
Copy link
Member

@traversaro what I wrote in #484 (comment) is still valid. I will open a new PR as soon as I manage to clean it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants