Port point cloud transformation to numpy #507

Flova · 2022-03-05T21:32:22Z

Implements the point cloud transformation in NumPy. This speeds up the transformation operation by 20000% for large point clouds (excluding the serialization and deserialization). The code needs some further testing but should work as it is. I also try to improve the performance of the serialization and deserialization as seen in ros2/common_interfaces#175.

This adds NumPy as a dependency and drops the dependency on PyKDL which should solve #471.

The API is also improved by adding proper interface descriptions.

Cheers

aprotyas · 2022-03-06T06:18:44Z

CI is failing here because it couldn't resolve the python-numpy rosdep key for Ubuntu Jammy. I believe the dependency needs to be on python3-numpy instead.

aprotyas

This looks like a great contribution. I don't know about adding numpy as a dependency, specially since upstream efforts for PyKDL are underway, but the maintainers can decide on that. I've left some suggestions inline.

Also, it looks like your commits aren't signed-off. Do you mind signing them off?
https://docs.ros.org/en/rolling/Contributing/Developer-Guide.html#change-control-process

tf2_sensor_msgs/src/tf2_sensor_msgs/tf2_sensor_msgs.py

tf2_sensor_msgs/package.xml

tf2_sensor_msgs/src/tf2_sensor_msgs/tf2_sensor_msgs.py

aprotyas · 2022-03-06T07:07:46Z

Also, is the DCO bot not configured on this repo for some reason? @clalancette

Flova · 2022-03-06T15:19:08Z

CI is failing here because it couldn't resolve the python-numpy rosdep key for Ubuntu Jammy. I believe the dependency needs to be on python3-numpy instead.

Done

Flova · 2022-03-06T15:56:04Z

There seems to be an issue with /usr/lib/x86_64-linux-gnu/libpython3.9.so on jammy.
I get the following error while building:

No rule to make target '/usr/lib/x86_64-linux-gnu/libpython3.9.so', needed by 'libstd_msgs__rosidl_generator_c.so'.  Stop.

And the CI fails with a similar one.

Flova · 2022-03-06T17:11:09Z

Sorry I needed to force push the new commit with signature.

Flova · 2022-03-10T01:25:36Z

All tests are passing. But I would want to wait for the serialization pull request, which would open some possibilities for improvements in this pr.

Any further suggestions regarding this implementation?

aprotyas

The PR looks good mostly. I've left some small comments in-line.

It might make sense to import the API in the scope of tf2_sensor_msgs/__init__.py with:

from .tf2_sensor_msgs import do_transform_cloud, transform_points  # noqa

tf2_sensor_msgs/CMakeLists.txt

tf2_sensor_msgs/test/test_tf2_sensor_msgs.py

tf2_sensor_msgs/tf2_sensor_msgs/tf2_sensor_msgs.py

aprotyas · 2022-03-29T08:06:38Z

@ros-pull-request-builder retest this please

tf2_sensor_msgs/package.xml

aprotyas · 2022-03-29T08:35:09Z

CI:
Build args: --packages-above-and-dependencies tf2_sensor_msgs
Test args: --packages-above tf2_sensor_msgs
Repos file: https://gist.githubusercontent.com/aprotyas/a62e4da45b1ec3f5d52838ff94617e94/raw/a61329ea9bfded0ef42a4d5ccae47b321d6dd937/ros2.repos
Job: https://ci.ros2.org/job/ci_launcher/10079

Linux
Linux-aarch64
Windows

aprotyas · 2022-03-29T19:46:01Z

@Flova looks like there are some linter errors in CI: can you address that?
Also, one of the tests fail because sensor_msgs_py can't be imported. I believe updating the list of dependencies (#507 (comment)) should sort that out.

clalancette · 2022-03-29T19:51:19Z

Also, it looks like some of the numpy changes from ros2/common_interfaces#175 caused regressions on RHEL: https://ci.ros2.org/view/nightly/job/nightly_linux-rhel_release/1091/ . I would suggest holding off on this PR until we figure out what is going on there and how to mitigate it (as we may need to do the same thing here).

aprotyas · 2022-03-29T22:34:55Z

Also, it looks like some of the numpy changes from ros2/common_interfaces#175 caused regressions on RHEL: https://ci.ros2.org/view/nightly/job/nightly_linux-rhel_release/1091/ . I would suggest holding off on this PR until we figure out what is going on there and how to mitigate it (as we may need to do the same thing here).

Related to ros2/common_interfaces#185, the numpy.typing module is introduced in numpy 1.20, and so will also cause regressions on RHEL.

Flova · 2022-03-30T08:54:21Z

Also, it looks like some of the numpy changes from ros2/common_interfaces#175 caused regressions on RHEL: https://ci.ros2.org/view/nightly/job/nightly_linux-rhel_release/1091/ . I would suggest holding off on this PR until we figure out what is going on there and how to mitigate it (as we may need to do the same thing here).

Related to ros2/common_interfaces#185, the numpy.typing module is introduced in numpy 1.20, and so will also cause regressions on RHEL.

Not only for RHEL. Afaik Ubuntu has a recent version that doesn't support it too. So i will remove the npt typing.

Flova · 2022-04-04T10:53:11Z

All TODOs should be resolved now. I added

the RHEL fix
optimizations if the cloud only includes coordinates
more tests for clouds that contain not only coordinates
more tests regarding the expected header as well as the registration in tf2

The rolling CI also passes, so we should be fine to rerun the tests for all architectures. @aprotyas
Maybe also have a look at my additions to the as it changed a bit.

Flova · 2022-04-19T10:09:07Z

Friendly ping @aprotyas :)

aprotyas

Apparently this was left as a pending review and I just never submitted - thank you for the ping @Flova.

This PR looks good. I've left a bunch of nits inline, but feel free to ignore them if they don't make sense.

Consider what constitutes the public API of tf2_sensor_msgs though, i.e. my comments in the __init__.py file.

tf2_sensor_msgs/tf2_sensor_msgs/__init__.py

tf2_sensor_msgs/tf2_sensor_msgs/tf2_sensor_msgs.py

tf2_sensor_msgs/test/test_tf2_sensor_msgs.py

Flova · 2022-04-21T08:54:09Z

@aprotyas everything should be resolved now

aprotyas

Thanks for iterating @Flova. I've left a couple of stray, minor nits that you can consider, but I'll approve regardless.

tf2_sensor_msgs/test/test_tf2_sensor_msgs.py

aprotyas · 2022-04-21T14:11:05Z

@Flova can you rebase your branch on the upstream HEAD? I can run CI for you once that's done.

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Signed-off-by: Florian Vahl <florian@flova.de>

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Signed-off-by: Florian Vahl <florian@flova.de>

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Signed-off-by: Florian Vahl <florian@flova.de>

aprotyas · 2022-04-21T21:52:24Z

Thanks for rebasing @Flova. Running CI now.

Repos file: https://gist.githubusercontent.com/aprotyas/25ed5c597ccf7fd55a64975b4a5c0abe/raw/0eb6fb2688926456e2944b39c07492898e9a8078/ros2.repos
Build args: --packages-above-and-dependencies tf2_sensor_msgs
Test args: --packages-above tf2_sensor_msgs
Job: https://ci.ros2.org/job/ci_launcher/10193/

Linux
Linux-aarch64
Windows

aprotyas · 2022-04-22T06:03:57Z

@Flova looks like there are some flake8 linter warnings, can you address them please? For the set comprehension warning, going from set(...) to {...} should suffice.

Signed-off-by: Florian Vahl <florian@flova.de>

Flova · 2022-04-22T07:20:10Z

@aprotyas I fixed the flake8 errors. Do you know why they only occur in this CI and not in the Jammy Build that is done automatically or my local installation?

aprotyas · 2022-04-22T13:28:04Z

@Flova it looks like the CI job installs these flake8 extensions which aren't present in the PR job. Same could apply for your local environment?

flake8==4.0.1
flake8-blind-except==0.1.1
flake8-builtins==1.5.3
flake8-class-newline==1.6.0
flake8-comprehensions==3.8.0
flake8-deprecated==1.3
flake8-docstrings==1.6.0
flake8-import-order==0.18.1
flake8-quotes==3.3.1

Re-running CI after 2cdf774.
Repos file: https://gist.githubusercontent.com/aprotyas/25ed5c597ccf7fd55a64975b4a5c0abe/raw/0eb6fb2688926456e2944b39c07492898e9a8078/ros2.repos
Build args: --packages-above-and-dependencies tf2_sensor_msgs
Test args: --packages-above tf2_sensor_msgs
Job: https://ci.ros2.org/job/ci_launcher/10194/

Linux
Linux-aarch64
Windows

aprotyas · 2022-04-26T18:47:06Z

@clalancette feature PRs can now be merged into Rolling again right? If so, this PR is ready to go.

clalancette

One more small thing to fix, then I think this is good.

tf2_sensor_msgs/package.xml

Signed-off-by: Florian Vahl <florian@flova.de>

Flova · 2022-04-26T21:11:58Z

The fix is applied. @clalancette

aprotyas

LGTM. Running CI again:

Repos file: https://gist.githubusercontent.com/aprotyas/25ed5c597ccf7fd55a64975b4a5c0abe/raw/0eb6fb2688926456e2944b39c07492898e9a8078/ros2.repos
Build args: --packages-above-and-dependencies tf2_sensor_msgs
Test args: --packages-above tf2_sensor_msgs
Job: https://ci.ros2.org/job/ci_launcher/10208/

Linux
Linux-aarch64
Windows

aprotyas · 2022-04-27T00:02:05Z

Green CI + two approvals, so I'll merge this. Thanks for your contribution @Flova!

Blast545 · 2022-04-28T14:04:05Z

👨‍🌾 Looks like this PR introduced a build regression in the cyclonedds jobs of the buildfarm, see:

Rci__nightly-cyclonedds_ubuntu_jammy_amd64#57
Rci__nightly-cyclonedds_ubuntu_jammy_amd64#58

01:33:42.101 -- Found ament_lint_auto: 0.12.3 (/tmp/ws/install_isolated/ament_lint_auto/share/ament_lint_auto/cmake)
01:33:42.101 CMake Error at CMakeLists.txt:41 (find_package):
01:33:42.101   By not providing "Findament_cmake_pytest.cmake" in CMAKE_MODULE_PATH this
01:33:42.101   project has asked CMake to find a package configuration file provided by
01:33:42.101   "ament_cmake_pytest", but CMake did not find one.
01:33:42.101 
01:33:42.101   Could not find a package configuration file provided by
01:33:42.101   "ament_cmake_pytest" with any of the following names:
01:33:42.101 
01:33:42.101     ament_cmake_pytestConfig.cmake
01:33:42.101     ament_cmake_pytest-config.cmake
01:33:42.101 
01:33:42.101   Add the installation prefix of "ament_cmake_pytest" to CMAKE_PREFIX_PATH or
01:33:42.101   set "ament_cmake_pytest_DIR" to a directory containing one of the above
01:33:42.101   files.  If "ament_cmake_pytest" provides a separate development package or
01:33:42.101   SDK, be sure it has been installed.
01:33:42.101

Can you take a look @Flova ?

Blast545 · 2022-04-28T14:12:39Z

Mmm I think it only requires adding ament_cmake_pytest to the package.xml file, I'll test this out and open a PR, we can discuss there.

aprotyas · 2022-04-28T14:13:33Z

Mmm I think it only requires adding ament_cmake_pytest to the package.xml file, I'll test this out and open a PR, we can discuss there.

@Blast545 #520 should fix it.

Blast545 · 2022-04-28T14:23:40Z

Just saw it, thanks! @aprotyas

aprotyas requested changes Mar 6, 2022

View reviewed changes

aprotyas mentioned this pull request Mar 6, 2022

Port pointcloud creation to numpy. ros2/common_interfaces#175

Merged

Flova force-pushed the ros2 branch 3 times, most recently from fdece22 to adac985 Compare March 6, 2022 17:10

Flova force-pushed the ros2 branch 2 times, most recently from e36337a to 14b378d Compare March 10, 2022 01:07

Flova requested a review from aprotyas March 15, 2022 14:59

Flova mentioned this pull request Mar 15, 2022

Drop PyKDL dependency in tf2_geometry_msgs #509

Merged

aprotyas reviewed Mar 29, 2022

View reviewed changes

tf2_sensor_msgs/CMakeLists.txt Outdated Show resolved Hide resolved

tf2_sensor_msgs/test/test_tf2_sensor_msgs.py Outdated Show resolved Hide resolved

tf2_sensor_msgs/tf2_sensor_msgs/tf2_sensor_msgs.py Outdated Show resolved Hide resolved

aprotyas reviewed Mar 29, 2022

View reviewed changes

tf2_sensor_msgs/package.xml Outdated Show resolved Hide resolved

Flova requested a review from aprotyas April 4, 2022 10:53

aprotyas requested changes Apr 20, 2022

View reviewed changes

aprotyas approved these changes Apr 21, 2022

View reviewed changes

tf2_sensor_msgs/test/test_tf2_sensor_msgs.py Outdated Show resolved Hide resolved

tf2_sensor_msgs/test/test_tf2_sensor_msgs.py Outdated Show resolved Hide resolved

tf2_sensor_msgs/test/test_tf2_sensor_msgs.py Outdated Show resolved Hide resolved

Flova and others added 11 commits April 21, 2022 23:38

Update tf2_sensor_msgs/test/test_tf2_sensor_msgs.py

5d1e120

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Update tf2_sensor_msgs/test/test_tf2_sensor_msgs.py

0adbbdf

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Update tf2_sensor_msgs/tf2_sensor_msgs/tf2_sensor_msgs.py

24f54ff

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Fix tests

d453ded

Signed-off-by: Florian Vahl <florian@flova.de>

Add authority to tf_buffer

d8936e6

Signed-off-by: Florian Vahl <florian@flova.de>

Update tf2_sensor_msgs/test/test_tf2_sensor_msgs.py

b3f981c

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Fix formating

67e3071

Signed-off-by: Florian Vahl <florian@flova.de>

Update tf2_sensor_msgs/test/test_tf2_sensor_msgs.py

8409953

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Update tf2_sensor_msgs/test/test_tf2_sensor_msgs.py

4b98025

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Update tf2_sensor_msgs/test/test_tf2_sensor_msgs.py

99b84b9

Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

Fix constant types

2a2aae2

Signed-off-by: Florian Vahl <florian@flova.de>

Flova force-pushed the ros2 branch from 4b9be33 to 2a2aae2 Compare April 21, 2022 21:38

Fix flake8 errors

2cdf774

Signed-off-by: Florian Vahl <florian@flova.de>

clalancette requested changes Apr 26, 2022

View reviewed changes

tf2_sensor_msgs/package.xml Outdated Show resolved Hide resolved

Make some deps exec_depend

0f436eb

Signed-off-by: Florian Vahl <florian@flova.de>

aprotyas approved these changes Apr 26, 2022

View reviewed changes

aprotyas merged commit 6242f01 into ros2:ros2 Apr 27, 2022

Blast545 mentioned this pull request May 10, 2022

👨‍🌾 tf2_sensor_msgs can't import numpy.core._multiarray_umath on Windows Debug #524

Closed

clalancette mentioned this pull request Nov 28, 2022

Issue with import tf2_sensor_msgs, create_cloud() and deleted "import rospy" #566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port point cloud transformation to numpy #507

Port point cloud transformation to numpy #507

Flova commented Mar 5, 2022 •

edited

Loading

aprotyas commented Mar 6, 2022

aprotyas left a comment •

edited

Loading

aprotyas commented Mar 6, 2022

Flova commented Mar 6, 2022

Flova commented Mar 6, 2022

Flova commented Mar 6, 2022

Flova commented Mar 10, 2022

aprotyas left a comment

aprotyas commented Mar 29, 2022

aprotyas commented Mar 29, 2022

aprotyas commented Mar 29, 2022

clalancette commented Mar 29, 2022

aprotyas commented Mar 29, 2022

Flova commented Mar 30, 2022

Flova commented Apr 4, 2022 •

edited

Loading

Flova commented Apr 19, 2022

aprotyas left a comment

Flova commented Apr 21, 2022

aprotyas left a comment

aprotyas commented Apr 21, 2022

aprotyas commented Apr 21, 2022

aprotyas commented Apr 22, 2022

Flova commented Apr 22, 2022

aprotyas commented Apr 22, 2022

aprotyas commented Apr 26, 2022

clalancette left a comment

Flova commented Apr 26, 2022

aprotyas left a comment

aprotyas commented Apr 27, 2022

Blast545 commented Apr 28, 2022

Blast545 commented Apr 28, 2022

aprotyas commented Apr 28, 2022

Blast545 commented Apr 28, 2022

Port point cloud transformation to numpy #507

Port point cloud transformation to numpy #507

Conversation

Flova commented Mar 5, 2022 • edited Loading

aprotyas commented Mar 6, 2022

aprotyas left a comment • edited Loading

Choose a reason for hiding this comment

aprotyas commented Mar 6, 2022

Flova commented Mar 6, 2022

Flova commented Mar 6, 2022

Flova commented Mar 6, 2022

Flova commented Mar 10, 2022

aprotyas left a comment

Choose a reason for hiding this comment

aprotyas commented Mar 29, 2022

aprotyas commented Mar 29, 2022

aprotyas commented Mar 29, 2022

clalancette commented Mar 29, 2022

aprotyas commented Mar 29, 2022

Flova commented Mar 30, 2022

Flova commented Apr 4, 2022 • edited Loading

Flova commented Apr 19, 2022

aprotyas left a comment

Choose a reason for hiding this comment

Flova commented Apr 21, 2022

aprotyas left a comment

Choose a reason for hiding this comment

aprotyas commented Apr 21, 2022

aprotyas commented Apr 21, 2022

aprotyas commented Apr 22, 2022

Flova commented Apr 22, 2022

aprotyas commented Apr 22, 2022

aprotyas commented Apr 26, 2022

clalancette left a comment

Choose a reason for hiding this comment

Flova commented Apr 26, 2022

aprotyas left a comment

Choose a reason for hiding this comment

aprotyas commented Apr 27, 2022

Blast545 commented Apr 28, 2022

Blast545 commented Apr 28, 2022

aprotyas commented Apr 28, 2022

Blast545 commented Apr 28, 2022

Flova commented Mar 5, 2022 •

edited

Loading

aprotyas left a comment •

edited

Loading

Flova commented Apr 4, 2022 •

edited

Loading