feat(lidar_centerpoint): enabled per class yaw norm threshold

[knzo25]

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

Description

The previous threshold to the yaw norm was applied equally to all classes.
This PR enables this step to use a dedicated threshold per class.

Pre-review checklist for the PR author

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

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.

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.

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

[codecov]

Codecov Report

Base: 10.37% // Head: 10.39% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (c5f0737) compared to base (248295e).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1962      +/-   ##
==========================================
+ Coverage   10.37%   10.39%   +0.02%     
==========================================
  Files        1159     1159              
  Lines       82453    82264     -189     
  Branches    19280    19290      +10     
==========================================
  Hits         8554     8554              
+ Misses      64639    64440     -199     
- Partials     9260     9270      +10     

Flag	Coverage Δ		*Carryforward flag
differential	0.00% <0.00%> (?)
total	10.39% <0.00%> (+0.03%)	⬆️	Carriedforward from 6125d20

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

Impacted Files	Coverage Δ
...include/ndt_scan_matcher/ndt_scan_matcher_core.hpp	0.00% <0.00%> (ø)
...can_matcher/include/ndt_scan_matcher/util_func.hpp	0.00% <ø> (ø)
localization/ndt_scan_matcher/src/debug.cpp	0.00% <0.00%> (ø)
...ion/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp	0.00% <0.00%> (ø)
localization/ndt_scan_matcher/src/util_func.cpp	0.00% <0.00%> (ø)
...ion_based_fusion/src/pointpainting_fusion/node.cpp	0.00% <0.00%> (ø)
...t/include/lidar_centerpoint/centerpoint_config.hpp	0.00% <0.00%> (ø)
...dar_centerpoint/postprocess/postprocess_kernel.hpp	0.00% <ø> (ø)
...lidar_centerpoint/lib/detection_class_remapper.cpp	0.00% <ø> (ø)
...rpoint/lib/postprocess/non_maximum_suppression.cpp	0.00% <ø> (ø)
... and 45 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.

[yukkysaito]

@knzo25 @yukke42 I have one question.

• What is yaw norm?

[knzo25]

@knzo25 @yukke42 I have one question.

• What is yaw norm?

Centerpoint by default estimates the rotation (yaw) via cosine/sine estimation + atan2.
However, there are no guarantees that the cosine/sine have unit norm (although centerpoint is trained so it does so).
In practice, the detections in which the norm of the cosine/sine estimation is low (e.g., cos=0.000 sin=0.001 -> yaw=90 degrees) are wrong so we filter them out

[yukke42]

I left a comment. Others look good.

[knzo25]

In addition to addressing your comment, I realized I made a mistake during the review of #1935.
In that PR, assert(params.target_class_names_.size() == 8); is no correct, since it contains the names of the classes subject to the algorithm, not the number of total classes.

I deleted said assert in the latest commit, but should I make a separate one-liner PR for that or is it OK to include it here?
Additionally, instead of constexpr std::size_t num_object_classification = 8;, shouldn't it be better to add a constant in the message file to make is usable everywhere?

[yukke42]

I deleted said assert in the latest commit, but should I make a separate one-liner PR for that or is it OK to include it here?

Thanks! Yes, It's ok.

Additionally, instead of constexpr std::size_t num_object_classification = 8;, shouldn't it be better to add a constant in the message file to make is usable everywhere?

I think we should add the constant value to the perception_utils package, but let's do it after #1954 since this is a big refactoring.

[yukke42]

LGTM. I check that there is no error.