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

Port pointcloud creation to numpy. #175

Merged
merged 20 commits into from
Mar 29, 2022
Merged

Port pointcloud creation to numpy. #175

merged 20 commits into from
Mar 29, 2022

Conversation

Flova
Copy link
Contributor

@Flova Flova commented Mar 5, 2022

This pull request ports the PointCloud creation to NumPy. It should massively increase the performance for large point clouds as no iteration over the points in python is needed and the bytes are taken directly from NumPy.
Still needs some work regarding other field types. Feel free to comment on my implementation. Suggestions are welcome.
Cheers

@aprotyas
Copy link
Member

aprotyas commented Mar 6, 2022

Repeating my comments from ros2/geometry2#507:

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.

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

@Flova
Copy link
Contributor Author

Flova commented Mar 6, 2022

Why is the DCO failing? Github shows my commits as correctly signed.

@aprotyas
Copy link
Member

aprotyas commented Mar 6, 2022

Why is the DCO failing? Github shows my commits as correctly signed.

It looks like you signed the commits with your GPG key, but you need to add a Signed-off-by line in your commit message.

Check here: https://docs.ros.org/en/rolling/Contributing/Developer-Guide.html#change-control-process

Signed-off-by: Florian Vahl <florian@flova.de>
Signed-off-by: Florian Vahl <florian@flova.de>
Signed-off-by: Florian Vahl <florian@flova.de>
…iple field datatypes

Signed-off-by: Florian Vahl <florian@flova.de>
Signed-off-by: Florian Vahl <florian@flova.de>
@Flova
Copy link
Contributor Author

Flova commented Mar 9, 2022

Duplicate of #155, but this PR has a bit more features and is activly worked on.

Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
… check fail because list!=tuple

Signed-off-by: Florian Vahl <git@flova.de>
@Flova
Copy link
Contributor Author

Flova commented Mar 9, 2022

While the CI still fails with some error related to python3.9 vs python3.10 in Jammy all tests are passing locally on my Ubuntu 20.04 ROS rolling machine.

@Flova
Copy link
Contributor Author

Flova commented Mar 9, 2022

Some performance testing:

As data, I used an unstructured NumPy array containing 480000 points.

Serialization using the old code:
700ms
Serialization using the updated code:
10ms
Deserialization using the old code:
140ms
Deserialization using the new code (read_points_numpy aka with cast to unstructured array):
143 µs (this is the correct unit)
Deserialization using the new code (read_points aka as structured array):
51.7 µs

@clalancette
Copy link
Contributor

While the CI still fails with some error related to python3.9 vs python3.10 in Jammy all tests are passing locally on my Ubuntu 20.04 ROS rolling machine.

Yeah, that's fine. This is currently broken across the board (we are waiting for some fixes in Jammy itself). Once we are closer to having this approved, we'll run CI on https://ci.ros2.org to see the status.

@Flova
Copy link
Contributor Author

Flova commented Mar 9, 2022

I plan to write some more tests regarding organized point clouds as well as ones with mixed dtypes.

Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
Signed-off-by: Florian Vahl <git@flova.de>
@Flova
Copy link
Contributor Author

Flova commented Mar 15, 2022

Any other comments regarding this PR? I think all mayor issues are addressed or moved upstream without affecting this PR.

Copy link
Member

@gbiggs gbiggs left a comment

Choose a reason for hiding this comment

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

LGTM pending passing CI.

@gbiggs
Copy link
Member

gbiggs commented Mar 24, 2022

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@gbiggs
Copy link
Member

gbiggs commented Mar 24, 2022

CI has picked up a small pile of flake8 linting errors.

08:12:16 _________________________________ test_flake8 __________________________________
08:12:16 test/test_flake8.py:38: in test_flake8
08:12:16     assert rc == 0, \
08:12:16 E   AssertionError: Found 20 code style errors / warnings:
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:42:1: I100 Import statements are in the wrong order. 'from collections import namedtuple' should be before 'import sys'
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:61:22: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:95:13: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:100:30: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:154:9: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:208:26: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:209:20: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:218:33: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:221:54: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:256:17: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:264:17: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:275:9: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:285:31: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./sensor_msgs_py/point_cloud2.py:286:31: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./test/test_point_cloud2.py:107:10: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./test/test_point_cloud2.py:108:10: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./test/test_point_cloud2.py:109:10: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./test/test_point_cloud2.py:228:51: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./test/test_point_cloud2.py:228:58: Q000 Double quotes found but single quotes preferred
08:12:16 E     ./test/test_point_cloud2.py:228:65: Q000 Double quotes found but single quotes preferred
08:12:16 E   assert 1 == 0

Signed-off-by: Florian Vahl <florian@flova.de>
@Flova
Copy link
Contributor Author

Flova commented Mar 27, 2022

@gbiggs I hopefully fixed all flake8 errors

@gbiggs
Copy link
Member

gbiggs commented Mar 27, 2022

Thanks! Another CI run:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Signed-off-by: Florian Vahl <florian@flova.de>
@Flova
Copy link
Contributor Author

Flova commented Mar 28, 2022

I'll definitely need to investigate why my flake8 config is so different to the one used by the CI. But I am now more or less confident that the imports are ordered correctly.

@gbiggs
Copy link
Member

gbiggs commented Mar 28, 2022

CI again:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@Flova
Copy link
Contributor Author

Flova commented Mar 29, 2022

Finally ^^

@gbiggs gbiggs merged commit 467f448 into ros2:master Mar 29, 2022
@gbiggs
Copy link
Member

gbiggs commented Mar 29, 2022

Thanks for the contribution!

aprotyas added a commit to ros2/geometry2 that referenced this pull request Apr 27, 2022
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.

Signed-off-by: Florian Vahl <florian@flova.de>
Co-authored-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
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

Successfully merging this pull request may close these issues.

None yet

6 participants