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_tvm): add lidar centerpoint tvm package #1898

Closed

Conversation

liuzf1988
Copy link

@liuzf1988 liuzf1988 commented Sep 16, 2022

Signed-off-by: zhengfei liu carl.liu@autocore.ai

Description

Provide another lidar centerpoint package using the TVM inference engine compared to the original one using TensorRT inference engine. And this package's directory structure is consistent the original package.

Related links

Related issue #908

Tests performed

Notes for reviewers

There is no problem with compiling, there may be some places that need further optimization.

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.

zhengfei liu and others added 2 commits September 16, 2022 16:17
@yukke42 yukke42 added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Sep 16, 2022
@angry-crab angry-crab self-requested a review September 20, 2022 15:34
@angry-crab
Copy link
Contributor

Thanks for the update. Could you briefly describe what problems we are having with this branch?

@liuzf1988
Copy link
Author

Thanks for the update. Could you briefly describe what problems we are having with this branch?

Ok, the current version of the code can be compiled, but it has not been tested with actual data. Now I'm recompiling the Autoware code, and later I will test this lidar_centerpoint_tvm package function and give the test results.

@liuzf1988
Copy link
Author

@angry-crab @ambroise-arm
When testing this package, it prompts "lidar_centerpoint_tvm: construct boxes3d failed". The prompt message comes from line 134 of lidar_centerpoint_tvm/lib/postprocess/generate_detected_boxes.cpp, indicating that no obstacles were detected. Currently I'm analyzing the reason for this prompt these days, and if possible, please help to find the reason. It may be that some of the functions of the code do not work.

lidar_centerpoint_tvm-testing-a

@angry-crab
Copy link
Contributor

@angry-crab @ambroise-arm When testing this package, it prompts "lidar_centerpoint_tvm: construct boxes3d failed". The prompt message comes from line 134 of lidar_centerpoint_tvm/lib/postprocess/generate_detected_boxes.cpp, indicating that no obstacles were detected. Currently I'm analyzing the reason for this prompt these days, and if possible, please help to find the reason. It may be that some of the functions of the code do not work.

lidar_centerpoint_tvm-testing-a

Thanks for the update! I'll look into it.

Copy link
Contributor

@ambroise-arm ambroise-arm left a comment

Choose a reason for hiding this comment

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

It may be that some of the functions of the code do not work.

I haven't finished reviewing everything, but what I have so far may help you.

@angry-crab
Copy link
Contributor

angry-crab commented Oct 3, 2022

generate_feature does not give correct output. I made a naive sequential version which output correctly. I'll try to figure out what's wrong with this one. Most likely, the indexing is wrong.

@angry-crab
Copy link
Contributor

@liuzf1988 I made some changes. Do you want me to add them directly to the pr or maybe as comments?

@liuzf1988
Copy link
Author

@liuzf1988 I made some changes. Do you want me to add them directly to the pr or maybe as comments?

Let me submit my changes first. If there are other changes that need to be made, you can modify the pr directly, OK?

zhengfei liu added 2 commits October 8, 2022 13:15
…d logic error

Signed-off-by: zhengfei liu <carl.liu@autocore.ai>
…d logic error

Signed-off-by: zhengfei liu <carl.liu@autocore.ai>
@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Base: 10.48% // Head: 10.47% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (9b408c8) compared to base (79fadf8).
Patch has no changes to coverable lines.

❗ Current head 9b408c8 differs from pull request most recent head 67b0dde. Consider uploading reports for the commit 67b0dde to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1898      +/-   ##
==========================================
- Coverage   10.48%   10.47%   -0.02%     
==========================================
  Files        1248     1252       +4     
  Lines       91218    91075     -143     
  Branches    21012    20919      -93     
==========================================
- Hits         9565     9538      -27     
- Misses      71466    71546      +80     
+ Partials    10187     9991     -196     
Flag Coverage Δ *Carryforward flag
differential ∅ <ø> (?)
total 10.44% <ø> (-0.02%) ⬇️ Carriedforward from 523b77f

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
...n/motion_utils/test/src/resample/test_resample.cpp 25.87% <0.00%> (-0.04%) ⬇️
common/osqp_interface/src/osqp_interface.cpp 33.65% <0.00%> (ø)
planning/route_handler/src/route_handler.cpp 0.00% <0.00%> (ø)
planning/rtc_interface/src/rtc_interface.cpp 0.00% <0.00%> (ø)
control/trajectory_follower/src/mpc_utils.cpp 58.04% <0.00%> (ø)
planning/behavior_velocity_planner/src/node.cpp 0.41% <0.00%> (ø)
common/motion_utils/src/marker/marker_helper.cpp 0.00% <0.00%> (ø)
localization/ekf_localizer/src/ekf_localizer.cpp 0.00% <0.00%> (ø)
control/vehicle_cmd_gate/src/vehicle_cmd_gate.cpp 0.29% <0.00%> (ø)
...anning/obstacle_velocity_limiter/src/obstacles.cpp 53.12% <0.00%> (ø)
... and 130 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@liuzf1988
Copy link
Author

Apart from some format adjustments, the bugs found have been fixed. After compiling, objects can be detected normally using this package, thank you both @angry-crab @ambroise-arm . And the screenshot of detection result is as follows:

lidar_centerpoint_tvm-test-results

Please modify the pr directly if other problems are found, thanks!

One more problem need to be discussed:
the code point[3] = timelag; in file voxel_generator.cpp means using "timelag" for the point's 4-th feature. However, according the paper, the point's 4-th feature should be the "intensity", am I right? If so, both the lidar_centerpoint and this package need to be modified.

@angry-crab
Copy link
Contributor

Apart from some format adjustments, the bugs found have been fixed. After compiling, objects can be detected normally using this package, thank you both @angry-crab @ambroise-arm . And the screenshot of detection result is as follows:

lidar_centerpoint_tvm-test-results

Please modify the pr directly if other problems are found, thanks!

One more problem need to be discussed: the code point[3] = timelag; in file voxel_generator.cpp means using "timelag" for the point's 4-th feature. However, according the paper, the point's 4-th feature should be the "intensity", am I right? If so, both the lidar_centerpoint and this package need to be modified.

@yukke42 Hi, do you know anything about why we are using timelag rather than intensity?

@angry-crab
Copy link
Contributor

angry-crab commented Oct 12, 2022

@liuzf1988 Cannot build the package. #1880 has to be merged first and changes has to be applied in this PR.

@angry-crab
Copy link
Contributor

angry-crab commented Oct 12, 2022

@liuzf1988 Unfortunately, generateFeature is still giving wrong output. Please take a look into it. I'm debugging it as well.

@angry-crab
Copy link
Contributor

angry-crab commented Oct 13, 2022

@liuzf1988 @ambroise-arm
After the latest request of changes being fixed, centerpoint_tvm should be able to work. However, there is still one issue.
I tried to replace cuda implementation with the cpp ones to verify its correctness. The detection outputs differ a little bit. Objects far from the vehicle cannot be recognized.

CUDA:
gpu

CPP:
cpu

In the images above, the same object cannot be correctly detected with cpp implementation. I think the problem is related to post processing part. We could open another issue to investigate further along with #1988 .

pre-commit-ci bot and others added 2 commits November 11, 2022 06:01
@github-actions github-actions bot added component:common Common packages from the autoware-common repository. (auto-assigned) document component:launch Launch files, scripts and initialization tools. (auto-assigned) labels Nov 15, 2022
@shmpwk shmpwk added type:documentation Creating or refining documentation. (auto-assigned) and removed document labels Nov 16, 2022
@ambroise-arm
Copy link
Contributor

Should we stick to pipleline class? Since it introduces overhead, maybe we can get rid of preprocess and postprocess, I feel like having InferenceEngineTVM is enough, just like TensorRT implementation of centerpoint. Further, for a single stage inference, the pipeline can be used. But for multi-stage inference, we have to handle data transfer between devices. The current pipeline design does not bring any benefit in such cases.

Possibly. But I don't have so much time to think about it for now.

Unrelated: this package will also need to align with #2234

Signed-off-by: Xinyu Wang <xinyu.wang@tier4.jp>
@github-actions github-actions bot removed the component:launch Launch files, scripts and initialization tools. (auto-assigned) label Nov 17, 2022
Copy link
Contributor

@ambroise-arm ambroise-arm left a comment

Choose a reason for hiding this comment

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

I haven't tried to run it.
The code looks good to me.

Signed-off-by: Xinyu Wang <xinyu.wang@tier4.jp>
@angry-crab angry-crab requested a review from a team as a code owner November 18, 2022 06:13
@angry-crab
Copy link
Contributor

angry-crab commented Nov 22, 2022

Something broke this package and there are conflicts. I'll open another one.

@angry-crab angry-crab closed this Nov 22, 2022
@ambroise-arm
Copy link
Contributor

ambroise-arm commented Nov 22, 2022

@angry-crab Would you be able to force-push to this branch instead of creating a new PR?

@angry-crab angry-crab reopened this Nov 25, 2022
@angry-crab
Copy link
Contributor

@angry-crab Would you be able to force-push to this branch instead of creating a new PR?

But I cannot rebase this PR since its not my repo. I'm not sure if it is fine. @mitsudome-r @xmfcx

@angry-crab
Copy link
Contributor

angry-crab commented Nov 28, 2022

@ambroise-arm
According to @mitsudome-r 's suggestion, I'll open another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:common Common packages from the autoware-common repository. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (auto-assigned) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants