Enhance iDynTree from / to Python mapping and add proper NumPy support #726

diegoferigo · 2020-08-27T12:15:21Z

After only one year from #484 (comment) I finally managed to fix and refactor that PR.

I don't know if it is a wise choice, but I tried to maintain the same APIs that @francesco-romano developed at that time. I implemented a somehow cleaner version of the original pull requests that 1) does not require to write any low-level CPython code and 2) exploits the existing typemaps provided by numpy.i. I cherry picked the original Python test.

Particularly, I exploited 2) in such a way that the memory of the objects mapped to numpy is handled by Python. When an object gets deleted, its memory is buffered for being garbage-collected [3].

Contrarily to #484, I used a mixture of C++ wrappers functions and Python extend in the SWIG file. Luckily, the recent refactoring and new inheritance scheme of #704 (see #704 (comment) for more details) helped reducing the cases.

Resources

CLAassistant · 2020-08-27T12:15:27Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ diegoferigo
❌ francesco-romano
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

traversaro · 2020-08-29T13:30:35Z

Luckily, the recent refactoring and new inheritance scheme of #704 (see #704 (comment) for more details) helped reducing the cases.

This PR is targeting master, where #704 did not landed for now.

traversaro · 2020-08-29T13:31:20Z

Tests on Ubuntu 18.04 are failing with:

 [ 98%] Building CXX object bindings/python/CMakeFiles/_iDynTree_python.dir/CMakeFiles/_iDynTree_python.dir/iDynTreePYTHON_wrap.cxx.o
/home/runner/work/idyntree/idyntree/build/bindings/python/CMakeFiles/_iDynTree_python.dir/iDynTreePYTHON_wrap.cxx:5515:10: fatal error: numpy/arrayobject.h: No such file or directory
 #include <numpy/arrayobject.h>
          ^~~~~~~~~~~~~~~~~~~~~

Based on https://packages.ubuntu.com/search?searchon=contents&keywords=numpy%2Farrayobject.h&mode=exactfilename&suite=bionic&arch=any I guess we need to either install python3-numpy, or pass some additional include directories to the compilation of the Python bindings.

traversaro · 2020-08-29T13:37:47Z

bindings/python/CMakeLists.txt

-  if(NOT IDYNTREE_USES_PYTHON_VERSION)
-     set (IDYNTREE_USES_PYTHON_VERSION ${PYTHON_VERSION_STRING})
-  endif()
+find_package(Python3 ${IDYNTREE_USES_PYTHON_VERSION} COMPONENTS Interpreter Development REQUIRED)


As long as this is targeting devel (i.e. iDynTree 2), it is ok to just support Python3 . In this context, what is the value of IDYNTREE_USES_PYTHON_VERSION ? Permit to find for different Python 3 version? I am not sure if FindPython3 works in this way.

If we can ignore Python2 support, I'll remove IDYNTREE_USES_PYTHON_VERSION entirely. If you want to find a specific version, you can use find_package(Python3 3.6 EXACT [...]), however this is not the case since we just need any Python3. Done in e6b6f50.

traversaro · 2020-08-29T13:37:57Z

bindings/python/CMakeLists.txt

-  get_filename_component(PYTHON_DIR ${PYTHON_LIBRARY} PATH)
-  link_directories(${PYTHON_DIR})
+if(NOT IDYNTREE_USES_PYTHON_VERSION)
+    set (IDYNTREE_USES_PYTHON_VERSION ${Python3_VERSION})


As IDYNTREE_USES_PYTHON_VERSION is a cache variable, this is either not doing anything, or in any case something of really confusing.

Right, in light of also #726 (comment) I removed it.

traversaro · 2020-08-29T13:40:16Z

Tests on Ubuntu 18.04 are failing with:
 [ 98%] Building CXX object bindings/python/CMakeFiles/_iDynTree_python.dir/CMakeFiles/_iDynTree_python.dir/iDynTreePYTHON_wrap.cxx.o
/home/runner/work/idyntree/idyntree/build/bindings/python/CMakeFiles/_iDynTree_python.dir/iDynTreePYTHON_wrap.cxx:5515:10: fatal error: numpy/arrayobject.h: No such file or directory
 #include <numpy/arrayobject.h>
          ^~~~~~~~~~~~~~~~~~~~~
Based on https://packages.ubuntu.com/search?searchon=contents&keywords=numpy%2Farrayobject.h&mode=exactfilename&suite=bionic&arch=any I guess we need to either install python3-numpy, or pass some additional include directories to the compilation of the Python bindings.

Checking a bit more the docs, I think we search and link the Python3::NumPy imported target ( https://cmake.org/cmake/help/v3.14/module/FindPython3.html#imported-targets ).

diegoferigo · 2020-08-30T10:05:59Z

Thanks @traversaro for the feedback!

This PR is targeting master, where #704 did not landed for now.

You're right, I rebased the PR on devel. I actually think that it could work also on master as it is, but if we can ignore python2 targeting devel maybe it's better doing so.

I think we search and link the Python3::NumPy imported target

I tried to compile and run the tests in my setup (bionic + virtualenv) and I didn't need to link that target. You're right that it's missing, maybe my system has those headers installed with apt in a already included directory. Done in 1d89ebe.

traversaro · 2020-08-30T10:45:46Z

Sorry, devel was behind master, and that is the reason why CI was failing (devel was missing #725), if you rebase on the top of latest devel everything should work fine.

from https://github.com/numpy/numpy/blob/529c61bdf69356b4a428d48bb5bc72c2417ed0cf/tools/swig/numpy.i

- From / to Numpy - From lists, tuples, ... for 1D vectors

diegoferigo · 2020-08-30T11:17:54Z

Thanks, rebased. Let's wait CI.

diegoferigo · 2020-08-31T12:54:37Z

@traversaro How should we handle the CLA? Either @francesco-romano signs it or I substitute the cherry-picked commit.

traversaro · 2020-08-31T13:06:26Z

I will force the check, as Francesco Romano already signed the CLA in another form.

traversaro · 2020-08-31T15:19:03Z

Can you update the CHANGELOG with the changes contained in this PR? Main thing to mention: dropped Python2 support, removed iDynTree.init_helpers()/iDynTree.init_numpy_helpers(), fromPyList, new features added.

diegoferigo · 2020-09-01T13:21:36Z

Updated the changelog in 0ad3c61.

I noticed that the dyncomp.py test is never added to the test suit, and it uses old classes that no longer exists and its syntax is Python2. I plan to use iDynTree bindings to create some high(er) level wrappers for KinDynComputations and InverseKinematics, let's see where to add the tests, either here or downstream.

traversaro · 2020-09-01T13:29:51Z

Thanks!

This is now require since robotology/idyntree#726 .

diegoferigo added Bindings: Python Component: Bindings Status: Feedback Complexity: Medium labels Aug 27, 2020

diegoferigo requested a review from traversaro August 27, 2020 12:15

diegoferigo self-assigned this Aug 27, 2020

diegoferigo force-pushed the feature/numpy branch from 5ae1586 to 1e42d1e Compare August 29, 2020 10:54

traversaro reviewed Aug 29, 2020

View reviewed changes

diegoferigo force-pushed the feature/numpy branch from 1e42d1e to 6e223fb Compare August 30, 2020 10:01

diegoferigo changed the base branch from master to devel August 30, 2020 10:02

diegoferigo force-pushed the feature/numpy branch from a42617c to e6b6f50 Compare August 30, 2020 10:13

diegoferigo added 3 commits August 30, 2020 13:16

Vendor numpy.i

3129a10

from https://github.com/numpy/numpy/blob/529c61bdf69356b4a428d48bb5bc72c2417ed0cf/tools/swig/numpy.i

Update Python bindings CMakeLists.txt

ff8b936

Enhance conversion iDynTree <--> Python

9643038

- From / to Numpy - From lists, tuples, ... for 1D vectors

diegoferigo force-pushed the feature/numpy branch from e6b6f50 to 2ec8883 Compare August 30, 2020 11:17

francesco-romano and others added 4 commits September 1, 2020 15:16

Updated Python bindings test files

6e407bc

Update helpers.py

980565e

Find and link Python bindings against NumPy target

9729a3b

Fix CMake variable in Python bindings tests

f205612

diegoferigo added 4 commits September 1, 2020 15:16

Remove IDYNTREE_USES_PYTHON_VERSION cached CMake variable

f9844b7

Install python3-numpy in Ubuntu CI

3dd823b

Update dyncomp.py test

4c1ec4e

Update changelog

0ad3c61

diegoferigo force-pushed the feature/numpy branch from 47812dd to 0ad3c61 Compare September 1, 2020 13:17

traversaro merged commit b4d8964 into robotology:devel Sep 1, 2020

traversaro added a commit to robotology/robotology-superbuild that referenced this pull request Sep 1, 2020

GitHub Actions: install python3-numpy

bd76226

This is now require since robotology/idyntree#726 .

traversaro mentioned this pull request Sep 1, 2020

GitHub Actions: install python3-numpy robotology/robotology-superbuild#458

Merged

diegoferigo deleted the feature/numpy branch September 1, 2020 21:44

diegoferigo restored the feature/numpy branch September 1, 2020 21:44

traversaro mentioned this pull request Sep 2, 2020

Permit to superbuild projects to require CMake 3.16 ? robotology/robotology-superbuild#462

Closed

This was referenced Sep 9, 2020

Initial support to Python packaging #733

Merged

Switch to use Python 3 by default #635

Closed

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

Open

diegoferigo mentioned this pull request Nov 20, 2020

Automatically publish wheels to PyPI #774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance iDynTree from / to Python mapping and add proper NumPy support #726

Enhance iDynTree from / to Python mapping and add proper NumPy support #726

diegoferigo commented Aug 27, 2020

CLAassistant commented Aug 27, 2020 •

edited

Loading

traversaro commented Aug 29, 2020

traversaro commented Aug 29, 2020

traversaro Aug 29, 2020

diegoferigo Aug 30, 2020

traversaro Aug 29, 2020

diegoferigo Aug 30, 2020

traversaro commented Aug 29, 2020

diegoferigo commented Aug 30, 2020 •

edited

Loading

traversaro commented Aug 30, 2020

diegoferigo commented Aug 30, 2020

diegoferigo commented Aug 31, 2020

traversaro commented Aug 31, 2020

traversaro commented Aug 31, 2020

diegoferigo commented Sep 1, 2020

traversaro commented Sep 1, 2020

Enhance iDynTree from / to Python mapping and add proper NumPy support #726

Enhance iDynTree from / to Python mapping and add proper NumPy support #726

Conversation

diegoferigo commented Aug 27, 2020

CLAassistant commented Aug 27, 2020 • edited Loading

traversaro commented Aug 29, 2020

traversaro commented Aug 29, 2020

traversaro Aug 29, 2020

Choose a reason for hiding this comment

diegoferigo Aug 30, 2020

Choose a reason for hiding this comment

traversaro Aug 29, 2020

Choose a reason for hiding this comment

diegoferigo Aug 30, 2020

Choose a reason for hiding this comment

traversaro commented Aug 29, 2020

diegoferigo commented Aug 30, 2020 • edited Loading

traversaro commented Aug 30, 2020

diegoferigo commented Aug 30, 2020

diegoferigo commented Aug 31, 2020

traversaro commented Aug 31, 2020

traversaro commented Aug 31, 2020

diegoferigo commented Sep 1, 2020

traversaro commented Sep 1, 2020

CLAassistant commented Aug 27, 2020 •

edited

Loading

diegoferigo commented Aug 30, 2020 •

edited

Loading