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

feat(lidar_centerpoint): accelerated preprocessing for centerpoint #6989

Merged

Conversation

knzo25
Copy link
Contributor

@knzo25 knzo25 commented May 12, 2024

Description

While changing the point type to the new standard, realized that preprocessing was bottleneck in centerpoint.
Apologies to Amadeusz since he did almost the same already in transfusion, and I forgot until writing this PR but I also moved the preprocessing to cuda for gpu acceleration.

During tests in my desktop the detection time went from 16.9ms to 13.0ms, making preprocessing about ~1ms if we forcefully add syncs to measure the time.

Related links

Tests performed

  • Tested using data from the taxi

Notes for reviewers

Interface changes

ROS Topic Changes

ROS Parameter Changes

Effects on system behavior

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25 knzo25 self-assigned this May 12, 2024
@github-actions github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label May 12, 2024
@knzo25 knzo25 changed the title feat: accelerated preprocessing for centerpoint feat(lidar_centerpoint): accelerated preprocessing for centerpoint May 12, 2024
@knzo25 knzo25 marked this pull request as ready for review May 13, 2024 04:09
@knzo25 knzo25 requested a review from kminoda as a code owner May 13, 2024 04:09
@knzo25
Copy link
Contributor Author

knzo25 commented May 13, 2024

@kminoda
This is by no means urgent, but it is always nice to reduce the perception latency.

if you do not feel comfortable reviewing this PR, Uetake-san, Hirabayashi-san, Tanaka-san, and Amadeusz are good candidates)

@kminoda kminoda added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label May 13, 2024
@kminoda
Copy link
Contributor

kminoda commented May 13, 2024

@knzo25 Thank you! And yes, I am still catching up with this package (especially the CUDA part), so would be nice if someone else could review it as a co-reviewer 🙏

@amadeuszsz Hi, would you help me out reviewing CUDA related code?

@kminoda
Copy link
Contributor

kminoda commented May 13, 2024

@knzo25 And also, would you execute Autoware Evaluator and verify that the basic scenarios would pass? Let me know if you don't know how to execute it.

Copy link
Contributor

@kminoda kminoda left a comment

Choose a reason for hiding this comment

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

Thank you for the awesome PR 🚀

@knzo25
Copy link
Contributor Author

knzo25 commented May 13, 2024

@kminoda
Regarding the evaluation I will look into it as soon as I can (got other stuff to deliver first)

@amadeuszsz
Copy link
Contributor

@amadeuszsz Hi, would you help me out reviewing CUDA related code?

@kminoda
Sure! I would like to make a review next week. Please let me know if it's urgent, I might change my priorities.
I guess you can't add me as a reviewer yet. I added my account to the contributors sheet, so I hope we will be able assign me soon.

@kminoda
Copy link
Contributor

kminoda commented May 14, 2024

@amadeuszsz Thank you. Not an urgent task, so please review this when you have time.

@amadeuszsz amadeuszsz self-requested a review May 31, 2024 02:15
Copy link
Contributor

@amadeuszsz amadeuszsz left a comment

Choose a reason for hiding this comment

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

Highly performance improvement observed! 👍🏻
Apart of minor changes requested, please check issues on your desktop with large clouds size & pointer arithmetic which causes wrong detections.

knzo25 added 4 commits June 3, 2024 15:06
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…imum capacity

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Copy link
Contributor Author

knzo25 commented Jun 3, 2024

Note to self: Pointpainting was using some of centerpoint parts as a base clase, so I will have to port some of the changes to pointpainting or duplicate logic for pointpainting

… new centerpoint logic is incompatible

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25
Copy link
Contributor Author

knzo25 commented Jun 3, 2024

@kminoda @YoshiRi @tzhong518
Since pointainting inherited some logic from centerpoint, and centerpoint now does things differently for GPU acceleration, pointpainting broke. For now, I just copied the logic from centerpoint before this PR and added it to the pointpainting package (the same optimization could not be applied easily).

If you also want pointpainting accelerated, let's us discuss / schedule it a later date please

@kminoda
Copy link
Contributor

kminoda commented Jun 3, 2024

@knzo25 Thanks. Still haven't understood in depth, but just to confirm: we would anyway have to implement different logic for centerpoint and pointpainting, right? I vote for merging this PR independently from pointpainting's further acceleration 👍

@amadeuszsz
Copy link
Contributor

From my side, lidar_centerpoint is ready to merge! 🚀 Please, confirm if the pointpainting discussion will be solved and we can proceed to merge.

@knzo25
Copy link
Contributor Author

knzo25 commented Jun 3, 2024

@kminoda
I will wait until the message migration happens before attempting to merge this. In addition I want to learn to use the evaluator so I will be trying that as soon as I get a respite from other things 🙏 (edit: now it is properly blocked xD)

knzo25 added 3 commits June 6, 2024 10:51
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

 Conflicts:
	perception/lidar_centerpoint/include/lidar_centerpoint/detection_class_remapper.hpp
	perception/lidar_centerpoint/include/lidar_centerpoint/node.hpp
	perception/lidar_centerpoint/include/lidar_centerpoint/ros_utils.hpp
…e code that assumed row-major, implements check for data layout, and updates tests

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@kminoda
Copy link
Contributor

kminoda commented Jun 10, 2024

@knzo25 Hi, would you fix the cppcheck error? (see cppcheck-differential result)

perception/lidar_centerpoint/include/lidar_centerpoint/network/tensorrt_wrapper.hpp:34:4: warning: inconclusive: Class 'TensorRTWrapper' which has virtual members does not have a virtual destructor. [virtualDestructor]
~TensorRTWrapper();

knzo25 added 3 commits June 10, 2024 15:21
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 3.21%. Comparing base (507e3f4) to head (0a8986f).
Report is 65 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #6989       +/-   ##
==========================================
- Coverage   14.84%   3.21%   -11.63%     
==========================================
  Files        1999     126     -1873     
  Lines      139163    8294   -130869     
  Branches    43716    1402    -42314     
==========================================
- Hits        20661     267    -20394     
+ Misses      95731    7971    -87760     
+ Partials    22771      56    -22715     
Flag Coverage Δ
differential 3.21% <ø> (?)
total ?

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@knzo25 knzo25 enabled auto-merge (squash) June 20, 2024 00:54
@knzo25 knzo25 merged commit fffb149 into autowarefoundation:main Jun 20, 2024
29 of 30 checks passed
@xmfcx
Copy link
Contributor

xmfcx commented Jun 24, 2024

@knzo25 starting from this PR, The CI for build-main started failing:

8: C++ exception with description "cudaErrorInsufficientDriver (35)@/__w/autoware.universe/autoware.universe/perception/lidar_centerpoint/include/lidar_centerpoint/cuda_utils.hpp#L80: CUDA driver version is insufficient for CUDA runtime version" thrown in the test body.

For some reason the has CI passed for the build-and-test-differential cuda yet it fails the build-and-test checks.

kminoda added a commit to kminoda/autoware.universe that referenced this pull request Jun 25, 2024
simon-eisenmann-driveblocks pushed a commit to simon-eisenmann-driveblocks/autoware.universe that referenced this pull request Jun 26, 2024
…utowarefoundation#6989)

* feat: accelerated preprocessing for centerpoint

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: changed includes to quotes instead of brackets

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: fixed variable name

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: addressed the case where the points in the cache is over the maximum capacity

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: fixed the offset of the output buffer in the kernel

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: reimplemented old logic in the pointpainting package since the new centerpoint logic is incompatible

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: eigen matrices are column major by default. this commit fixes the code that assumed row-major, implements check for data layout, and updates tests

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: fixed missing virtual destructor

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

---------

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Co-authored-by: kminoda <44218668+kminoda@users.noreply.github.com>
Signed-off-by: Simon Eisenmann <simon.eisenmann@driveblocks.ai>
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
…6989)

* feat: accelerated preprocessing for centerpoint

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: changed includes to quotes instead of brackets

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: fixed variable name

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: addressed the case where the points in the cache is over the maximum capacity

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: fixed the offset of the output buffer in the kernel

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: reimplemented old logic in the pointpainting package since the new centerpoint logic is incompatible

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: eigen matrices are column major by default. this commit fixes the code that assumed row-major, implements check for data layout, and updates tests

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: fixed missing virtual destructor

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

---------

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Co-authored-by: kminoda <44218668+kminoda@users.noreply.github.com>
TomohitoAndo pushed a commit to tier4/autoware.universe that referenced this pull request Sep 19, 2024
…utowarefoundation#6989)

* feat: accelerated preprocessing for centerpoint

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: changed includes to quotes instead of brackets

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: fixed variable name

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: addressed the case where the points in the cache is over the maximum capacity

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: fixed the offset of the output buffer in the kernel

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: reimplemented old logic in the pointpainting package since the new centerpoint logic is incompatible

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: eigen matrices are column major by default. this commit fixes the code that assumed row-major, implements check for data layout, and updates tests

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: fixed missing virtual destructor

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

---------

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Co-authored-by: kminoda <44218668+kminoda@users.noreply.github.com>
knzo25 added a commit to tier4/autoware.universe that referenced this pull request Dec 16, 2024
…utowarefoundation#6989)

* feat: accelerated preprocessing for centerpoint

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: changed includes to quotes instead of brackets

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: fixed variable name

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: addressed the case where the points in the cache is over the maximum capacity

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: fixed the offset of the output buffer in the kernel

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: reimplemented old logic in the pointpainting package since the new centerpoint logic is incompatible

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* fix: eigen matrices are column major by default. this commit fixes the code that assumed row-major, implements check for data layout, and updates tests

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

* chore: fixed missing virtual destructor

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>

---------

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Co-authored-by: kminoda <44218668+kminoda@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:perception Advanced sensor data processing and environment understanding. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants