feat(extrinsic_reflector_based_calibrator): add metric plotter for cross validation and add deletion button

[vividf]

Description

This PR adds three new features to the radar-lidar calibrator.

1. Add an additional cross-validation metric for evaluation.
2. Add a delete button on the UI for the user to delete the previous radar-lidar pair.
3. Add a new plotter to visualize the metrics in real time.

Related links

Tests performed

Notes for reviewers

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

[knzo25]

Thanks for the PR 🎉
I will be checking this PR around next week, but in the meantime can you check that the CI/CD passes?
(galactic is not needed)

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 183 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (21a1158) to head (61ef8f7).
Report is 44 commits behind head on tier4/universe.

Files with missing lines	Patch %	Lines
...rator/src/extrinsic_reflector_based_calibrator.cpp	0.00%	183 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##           tier4/universe    #145      +/-   ##
=================================================
- Coverage            0.95%   0.00%   -0.96%     
=================================================
  Files                 269       6     -263     
  Lines               20905     952   -19953     
  Branches              383       0     -383     
=================================================
- Hits                  200       0     -200     
+ Misses              20548     952   -19596     
+ Partials              157       0     -157     

Flag	Coverage Δ
differential	0.00% <0.00%> (?)
total	?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[knzo25]

Just in case no one told you before. The spellchecker currently fails in the following words:

Warning: Unknown word (forground)
sensor/extrinsic_reflector_based_calibrator/rviz/x2_front_center.rviz:129:19 Unknown word (forground)
sensor/extrinsic_reflector_based_calibrator/scripts/metrics_plotter_node.py:27:44 Unknown word (nrows)
sensor/extrinsic_reflector_based_calibrator/scripts/metrics_plotter_node.py:27:[53](https://github.com/tier4/CalibrationTools/actions/runs/7044433176/job/19172099781?pr=145#step:3:59) Unknown word (ncols)
sensor/extrinsic_reflector_based_calibrator/scripts/metrics_plotter_node.py:86:16 Unknown word (annos)
sensor/extrinsic_reflector_based_calibrator/scripts/metrics_plotter_node.py:227:21 Unknown word (annos)
sensor/extrinsic_reflector_based_calibrator/src/extrinsic_reflector_based_calibrator.cpp:1661:7 Unknown word (ncrossval)
sensor/extrinsic_reflector_based_calibrator/src/extrinsic_reflector_based_calibrator.cpp:1662:7 Unknown word (ncrossval)

Putting aside the one that are misspells, abbreviations are marked as unknown by our current dictionary.
For example nrows -> number_of_rows or rows
In the case we are using external libraries, we can use tags in the code to disable the spell checker in that line
In the case we are actually using conventions that are better to keep as is, we can add the words in the dictionary: https://github.com/tier4/CalibrationTools/blob/tier4/universe/.cspell.json

[knzo25]

The CalibrationTools s full of misspells, which are going to be fixed in our big next task. They were not caught at the proper time since the dictionary was added after the code was written

[knzo25]

@vividf
General comments first:
The plot works as expected but there are several small details I would like to be addressed.

1. At the beginning, the matplotlib window covers all the screen, I would like it to have a small initial resolution so that it does not bother the user.
2. At the beginning, the number of reflectors are not enforced to be integers. Please add that condition
3. Although the tool is called reflector-based, it was to make it clear that it is different that our other tag based tools. Please call it marker in the UI
4. Please add a suitable name to the figure instead of "Figure 1"
5. The distance units in ROS is meters. However, for evaluation purposes, it is more convenient to have it in centimeters in the UI. We report the error in centimeters. In the same way, the value ad the end of the plot is a good idea, but when converted to cm, please fix the decimals to the milimeter. Our sensors don't go beyond that anyway.
6. Avoid abbreviations when possible to make it as clear as possible:
   • "# reflector" -> "number of reflectors"
   • "Cross val error: D..." -> "Cross-validation error: d"
7. As you can see in the attached image, the axis of the upper image appears in the same position that the title of the lower plot. Please make sure that that does not happen (this may be resolution dependent though)

[knzo25]

[knzo25]

About the lifetime of markers.
The text marker should have infinite lifetime.
The other day I told you that the error of the converged tracks markers not disappearing was due to its infinite lifetime.
However, though giving the marker a finite lifetime is not the solution.
In this case what you need to do is send a deletion command via the marker topic. Example marker.action = visualization_msgs::Marker::DELETE;

[knzo25]

@vividf
Are using pre-commit locally? ci/cd keeps complaining it seems

[vividf]

Yes, I run it. But seems that I forget to commit it 😅

[knzo25]

@vividf
What do you mean you forget to commit?
If you installed it correctly, it should not allow you to commit things that do not pass its tests

[vividf]

I commit my code but didn't push. And I remember to run precommit. So I run the precommit run -a. There are some error in other code but not mine and it autofix my code (just like what GitHub does) but after that I forget to commit the changes and I push.😅

[knzo25]

@vividf
I don't know how everyone else is doing it by if you execute pre-commit install, then every git commit -s you execute afterwards will automatically run pre-commit so there are no chances your commit will violate the linters and stuff

[vividf]

I see. I was taught by another way. Thank you! I will run the -s in the future.

[knzo25]

@vividf
The -s is for another reason completely.
Even if you run git commit the pre-commit will still run.
-s is for signing your commits (I think David mentioned something to you along the lines the other day)

[knzo25]

(signing the commits is an absoluute rule in other Tier IV repositories)

[vividf]

Understand! I am sorry😓 and thanks for reminding me again.

[knzo25]

No biggie 👍
Firsts PRs in a new organization always come with lots of things to learn

[knzo25]

Awesome work. Only minor details for this round 💪

[knzo25]

LGMT!
@vividf
When you merge, make sure you use SQUASH AND MERGE.
Otherwise someone will come and complain :)