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_transfusion): add lidar_transfusion 3D detection package #6890

Merged
merged 60 commits into from
Jun 7, 2024

Conversation

amadeuszsz
Copy link
Contributor

@amadeuszsz amadeuszsz commented Apr 25, 2024

Description

The PR adds a new lidar_transfusion package which brings a new 3D detection component based on TransFusion[1].

lidar_transfusion_demo-2024-04-25_15.56.14.mp4

Related links

[1] Xuyang Bai, Zeyu Hu, Xinge Zhu, Qingqiu Huang, Yilun Chen, Hongbo Fu and Chiew-Lan Tai. "TransFusion: Robust LiDAR-Camera Fusion for 3D Object Detection with Transformers." arXiv preprint arXiv:2203.11496 (2022)
[2] rosbag (TIER IV internal link)
[3] helper files (TIER IV internal link)
[4] TransFusion onnx model (TIER IV internal link)

Tests performed

  • Model metrics:
------------- T4Metric results -------------
04/25 09:28:12 - mmengine - INFO - | class_name | mAP   | [email protected] | [email protected] | [email protected] | [email protected] | error@trans_err | error@scale_err | error@orient_err | error@vel_err | error@attr_err |
04/25 09:28:12 - mmengine - INFO - |----------------------------------------------------------------------------------------------------------------------------------------------------|
04/25 09:28:12 - mmengine - INFO - | car        | 0.788 | 0.609   | 0.798   | 0.859   | 0.884   | 0.26            | 0.159           | 0.103            | 0.779         | 1              |
04/25 09:28:12 - mmengine - INFO - | truck      | 0.59  | 0.315   | 0.577   | 0.7     | 0.766   | 0.379           | 0.149           | 0.0516           | 0.77          | 1              |
04/25 09:28:12 - mmengine - INFO - | bus        | 0.554 | 0.347   | 0.532   | 0.628   | 0.709   | 0.363           | 0.137           | 0.0762           | 1.69          | 1              |
04/25 09:28:12 - mmengine - INFO - | bicycle    | 0.236 | 0.192   | 0.241   | 0.251   | 0.261   | 0.283           | 0.248           | 0.27             | 0.839         | 1              |
04/25 09:28:12 - mmengine - INFO - | pedestrian | 0.628 | 0.57    | 0.613   | 0.642   | 0.688   | 0.253           | 0.281           | 0.424            | 0.626         | 1              |
04/25 09:28:12 - mmengine - INFO - | Total mAP: 0.559
  • ROS 2 node performance with RTX 4090:
                   | pre-process [ms] | inference [ms] | post-process [ms] | total [ms]  |
------------------------------------------------------------------------------------------
lidar_transfusion  |   1.52 ± 0.14    |  2.29 ± 0.37   |    0.20 ± 0.15    | 4.07 ± 0.44 |
lidar_centerpoint  |        -         |       -        |         -         | 5.53 ± 0.55 |

Notes for reviewers

The package can best tested with rosbag file. If data needed, you can use this rosbag[2] and copy helper files[3] to the launch directory of lidar_transfusion package before building. The default path for onnx model is ~/autoware_data/lidar_transfusion/transfusion.onnx. The model awaits for deployment, temporary please use attached link[4].

To start, run commands:

# terminal 1
ros2 launch lidar_transfusion data_pipeline.launch.py 

# terminal 2
ros2 bag play trailer_yaw_ticket_data_jpntaxi7/01d060d9-8d25-45e0-b45e-9fd7201ac27b_2023-05-26-11-30-03_p0900_3.db3 --loop --clock

# terminal 3
rviz2 --ros-args -p use_sim_time:=True 

# terminal 4 (for convenience you can add <param name="use_sim_time" value="true"/> to the node in xml file)
ros2 launch lidar_transfusion lidar_transfusion.launch.xml

Interface 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.

@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (auto-assigned) labels Apr 25, 2024
@amadeuszsz amadeuszsz marked this pull request as ready for review April 25, 2024 10:25
@scepter914 scepter914 self-requested a review April 26, 2024 01:21
@knzo25 knzo25 self-requested a review April 26, 2024 01:45
@knzo25
Copy link
Contributor

knzo25 commented May 2, 2024

Comments:

  • When the number of voxels falls outside the expected parameters, the output is 200 NaN objects (200 being the max number of objects). This can happen when the top lidar is missing in the taxi, for example. We should consider lowering the value, and/or aborting the inference + adding an error message.
  • We need to add transfusion to the perception related launchers. I did it in my local environment, so I will either pass them to the PR's author, commit them myself, or explain him the procedure.
  • In my environment, I have cuda memory errors sporadically. It happens inside the first kernel of the preprocess step.
  • Not confirmed, but the stream used in the preprocessing may not be the one we are expecting (since adding device synchronizations instead of streams changed the behavior)

@knzo25
Copy link
Contributor

knzo25 commented May 2, 2024

@scepter914
The PR can still be reviewed to some degree, but I think it is better to wait until the errors disappear 🙏

@taikitanaka3
Copy link
Contributor

@knzo25
I think you wanted to mention @scepter914

@knzo25
Copy link
Contributor

knzo25 commented May 2, 2024

Apologies 🙇

@amadeuszsz
Copy link
Contributor Author

Thank you @knzo25 for your review. Let me investigate the kernels first to find mentioned leak. Regarding the NaN output, as you suggested we will:

  • change the optimization profile,
  • handle the cases with not sufficient number of voxels after preprocessing for optimization profile.

@amadeuszsz
Copy link
Contributor Author

@knzo25
Recent fix solves the issue with cuda memory and NaNs. Please check if the issue disappeared on your machine as well.

amadeuszsz and others added 2 commits May 2, 2024 20:20
Copy link
Contributor

@scepter914 scepter914 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 PR.
Overall very readable code and rich documentation is so great. 👍

I took a quick look at whole code and I ask you to add some members for maintainers because development will be robuster and smoother if multiple people can maintain it.
As the rest of my work, I'll test with some rosbag and I'll approve after I confirm the operation with some Rosbag.

perception/lidar_transfusion/package.xml Outdated Show resolved Hide resolved
@scepter914
Copy link
Contributor

@amadeuszsz

I apologize for the inconvenience, would you fix DCO?
We need to pass CI include DCO for merge (this specification is so invonvenient...).

Co-authored-by: Satoshi Tanaka <[email protected]>

Signed-off-by: amadeuszsz <[email protected]>
@amadeuszsz amadeuszsz force-pushed the feat/lidar_transfusion branch from 8506f8a to f5e8146 Compare May 8, 2024 06:50
amadeuszsz and others added 2 commits May 8, 2024 19:22
Copy link
Contributor

@scepter914 scepter914 left a comment

Choose a reason for hiding this comment

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

I confirm the perform with other rosbags.
After kenzo-san's review and uploading model, we can merge this PR. 👍

@amadeuszsz amadeuszsz requested a review from miursh as a code owner May 10, 2024 01:27
@amadeuszsz amadeuszsz disabled auto-merge June 7, 2024 06:11
@amadeuszsz amadeuszsz enabled auto-merge (squash) June 7, 2024 06:11
@amadeuszsz amadeuszsz disabled auto-merge June 7, 2024 06:21
@amadeuszsz amadeuszsz enabled auto-merge (squash) June 7, 2024 06:22
@amadeuszsz amadeuszsz disabled auto-merge June 7, 2024 06:33
@amadeuszsz amadeuszsz enabled auto-merge (squash) June 7, 2024 06:34
@amadeuszsz amadeuszsz disabled auto-merge June 7, 2024 06:53
@amadeuszsz amadeuszsz enabled auto-merge (squash) June 7, 2024 06:53
@amadeuszsz amadeuszsz disabled auto-merge June 7, 2024 07:07
@YoshiRi YoshiRi disabled auto-merge June 7, 2024 11:10
@knzo25 knzo25 enabled auto-merge (squash) June 7, 2024 11:11
@amadeuszsz amadeuszsz disabled auto-merge June 7, 2024 12:38
@amadeuszsz amadeuszsz enabled auto-merge (squash) June 7, 2024 12:39
@knzo25 knzo25 added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jun 7, 2024
@amadeuszsz amadeuszsz merged commit d891ef7 into autowarefoundation:main Jun 7, 2024
32 of 34 checks passed
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
…6890)

* feat(lidar_transfusion): add lidar_transfusion 3D detection package

Signed-off-by: amadeuszsz <[email protected]>

* style(pre-commit): autofix

* style(lidar_transfusion): cpplint

Signed-off-by: amadeuszsz <[email protected]>

* style(lidar_transfusion): cspell

Signed-off-by: Amadeusz Szymko <[email protected]>

* fix(lidar_transfusion): CUDA mem allocation & inference input

Signed-off-by: amadeuszsz <[email protected]>

* style(pre-commit): autofix

* fix(lidar_transfusion): arrays size

Signed-off-by: amadeuszsz <[email protected]>

* style(pre-commit): autofix

* chore(lidar_transfusion): update maintainers

Co-authored-by: Satoshi Tanaka <[email protected]>

Signed-off-by: amadeuszsz <[email protected]>

* fix(lidar_transfusion): array size & grid idx

Signed-off-by: amadeuszsz <[email protected]>

* chore(lidar_transfusion): update maintainer email

Signed-off-by: amadeuszsz <[email protected]>

* chore: added transfusion to the respective launchers

Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>

* refactor(lidar_transfusion): rename config

Signed-off-by: amadeuszsz <[email protected]>

* refactor(lidar_transfusion): callback access specifier

Signed-off-by: amadeuszsz <[email protected]>

* refactor(lidar_transfusion): pointers initialziation

Signed-off-by: amadeuszsz <[email protected]>

* refactor(lidar_transfusion): change macros for constexpr

Signed-off-by: amadeuszsz <[email protected]>

* refactor(lidar_transfusion): consts & uniform initialization

Signed-off-by: amadeuszsz <[email protected]>

* refactor(lidar_transfusion): change to unique ptr & uniform initialization

Signed-off-by: amadeuszsz <[email protected]>

* style(pre-commit): autofix

* refactor(lidar_transfusion): use of config params

Signed-off-by: amadeuszsz <[email protected]>

* refactor(lidar_transfusion): remove unnecessary condition

Signed-off-by: amadeuszsz <[email protected]>

* style(lidar_transfusion): switch naming (CPU to HOST)

Signed-off-by: amadeuszsz <[email protected]>

* refactor(lidar_transfusion): remove redundant device sync

Signed-off-by: amadeuszsz <[email protected]>

* style(lidar_transfusion): intensity naming

Signed-off-by: amadeuszsz <[email protected]>

* feat(lidar_transfusion): full network shape validation

Signed-off-by: amadeuszsz <[email protected]>

* feat(lidar_transfusion): validate objects' orientation in host processing

Signed-off-by: amadeuszsz <[email protected]>

* feat(lidar_transfusion): add json schema

Signed-off-by: amadeuszsz <[email protected]>

* style(pre-commit): autofix

* style(lidar_transfusion): affine matrix naming

Signed-off-by: amadeuszsz <[email protected]>

* style(lidar_transfusion): transformed point naming

Signed-off-by: amadeuszsz <[email protected]>

* refactor(lidar_transfusion): add param descriptor & arrays size check

Signed-off-by: amadeuszsz <[email protected]>

* style(lidar_transfusion): affine matrix naming

Signed-off-by: amadeuszsz <[email protected]>

* feat(lidar_transfusion): caching cloud input as device ptr

Signed-off-by: amadeuszsz <[email protected]>

* fix(lidar_transfusion): logging

Signed-off-by: amadeuszsz <[email protected]>

* chore(tier4_perception_launch): revert to centerpoint

Signed-off-by: amadeuszsz <[email protected]>

* fix(lidar_transfusion): typo

Signed-off-by: amadeuszsz <[email protected]>

* docs(lidar_transfusion): use hook for param description

Signed-off-by: amadeuszsz <[email protected]>

* fix(lidar_transfusion): interpret eigen matrix as col major

Signed-off-by: amadeuszsz <[email protected]>

* feat(lidar_transfusion): update to autware_msgs

Signed-off-by: amadeuszsz <[email protected]>

---------

Signed-off-by: Amadeusz Szymko <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Kenzo Lobos Tsunekawa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:launch Launch files, scripts and initialization tools. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants