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(calibration_tools): new api and 2.0 release #148

Merged
merged 55 commits into from
Apr 11, 2024

Conversation

knzo25
Copy link
Collaborator

@knzo25 knzo25 commented Dec 28, 2023

Description

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.

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.

yabuta and others added 7 commits July 6, 2022 16:18
copy calibration tools from private repository
merge main for galactic release
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Added an option to use rectified images
Deleted the raw sqpnp method since it is included in the standard opencv now
Enabled the use of hardcoded system time (receiving time) for when sensors are now synchronized
Parameterized rviz profiles to avoid having multiple profiles (for this method one should be enough for all use cases)
Exposed the number of pnp pairs for calibration and the distance between calibration data and new detections to ease the work of field engineers
Changed timers since the default work now (for years now though)
Still have not done the spell checking

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25 knzo25 self-assigned this Dec 28, 2023
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…& base-lidar calibrators

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25 knzo25 requested a review from vividf January 10, 2024 04:51
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…mentation of a native way to edit the launchers

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…mentation of a native way to edit the launchers

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…into feature/new_api

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…w parameters ant the new launcher configuration schmeme

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…tion for the rdv, and others

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

knzo25 commented Jan 15, 2024

@yabuta
nedbat/coveragepy#1392
pytest-dev/pytest-cov#539

As in the previous links, CI/CD is failing regardless of the code (it looks for non-existing files). What happens in this case? Ignore CI/CD? Disable the offending ones? Will you look for a fix?

@yabuta
Copy link
Collaborator

yabuta commented Jan 15, 2024

@knzo25
Sorry, I couldn't figure out where in this PR CI/CD the above error is occurring. Is it happening in build-and-test-differential / build-and-test-differential (humble) (pull_request)?

@knzo25
Copy link
Collaborator Author

knzo25 commented Jan 15, 2024

@yabuta

The error is as follows:

2024-01-15T01:00:11.4597855Z Starting >>> new_extrinsic_calibration_manager 2024-01-15T01:00:13.0596455Z --- stderr: new_extrinsic_calibration_manager 2024-01-15T01:00:13.0597974Z --- output: new_extrinsic_calibration_manager 2024-01-15T01:00:13.0600676Z Warning: Couldn't parse '/__w/CalibrationTools/CalibrationTools/sensor/new_extrinsic_calibration_manager/signature_bootstrap.py': No source for code: '/__w/CalibrationTools/CalibrationTools/sensor/new_extrinsic_calibration_manager/signature_bootstrap.py'. (couldnt-parse) 2024-01-15T01:00:13.0603030Z --- 2024-01-15T01:00:13.0603624Z Failed <<< new_extrinsic_calibration_manager [1.60s, exited with code 2] 2024-01-15T01:00:13.0606444Z Warning: Couldn't parse '/__w/CalibrationTools/CalibrationTools/sensor/new_extrinsic_calibration_manager/signature_bootstrap.py': No source for code: '/__w/CalibrationTools/CalibrationTools/sensor/new_extrinsic_calibration_manager/signature_bootstrap.py'. (couldnt-parse)

Looking for the file signature_bootstrap.py on google led me to the links I posted before. Note that that does not happen at the build stage but the test one. Just in case, I commented out the test dependencies in the packages.xml to see it we could handle it that way temporarily but no luck

@yabuta
Copy link
Collaborator

yabuta commented Jan 15, 2024

@knzo25 I'll check, but if I can't resolve the cause, I'll try to disable CICD when you merge the PRs.
Or does this error affect your development?

@knzo25
Copy link
Collaborator Author

knzo25 commented Jan 15, 2024

@yabuta
It does not affect the development, but I don't really know who is in charge of CI/CD so in the meantime I just reported it here.
When the time for the merge comes, if there has not been a solution, I will drop a mention so we can disable and merge

…d bugs got discovered while doing do. Need to integrate all the options and the products

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
…ed in the xx1. can use the initial calibration as a fixed ground plane

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
knzo25 and others added 2 commits March 19, 2024 15:21
Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25 knzo25 requested a review from vividf March 19, 2024 06:24
@knzo25
Copy link
Collaborator Author

knzo25 commented Apr 2, 2024

@vividf
As a reminder:

  • As I mentioned a few weeks ago, the ball in on your court now
  • If you start a comment / review, if the answer is satisfactory, you need to click resolve. If it is not, you need to give the appropriate feedback.
  • As we saw during the taxi experiments, there are changes you did not comprehend. While there is no need to understand the whole codebase (unless you want to), you need to understand all the changes. If you need to study parts of the code to understand the changes, then that is also part of the code review. How much one needs to study the codebase to make a meaningful review depends on the person

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

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

codecov-commenter commented Apr 2, 2024

Codecov Report

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

Project coverage is 5.09%. Comparing base (bab5400) to head (45453cf).
Report is 3 commits behind head on tier4/universe.

Files Patch % Lines
...ier4_ground_plane_utils/src/ground_plane_utils.cpp 0.00% 122 Missing ⚠️
common/tier4_tag_utils/src/lidartag_hypothesis.cpp 0.00% 101 Missing ⚠️
common/tier4_tag_utils/src/apriltag_hypothesis.cpp 0.00% 32 Missing ⚠️
common/tier4_tag_utils/src/apriltag_filter.cpp 0.00% 10 Missing ⚠️
common/tier4_tag_utils/src/lidartag_filter.cpp 0.00% 10 Missing ⚠️
...de/tier4_ground_plane_utils/ground_plane_utils.hpp 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                Coverage Diff                @@
##           tier4/universe    #148      +/-   ##
=================================================
+ Coverage            0.93%   5.09%   +4.15%     
=================================================
  Files                 270     176      -94     
  Lines               21339   13055    -8284     
  Branches              383    1416    +1033     
=================================================
+ Hits                  200     665     +465     
+ Misses              20982   12364    -8618     
+ Partials              157      26     -131     
Flag Coverage Δ
differential 5.09% <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.

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

@vividf vividf left a comment

Choose a reason for hiding this comment

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

After the changes are updated I think it looks good for me to merge 2.0

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25 knzo25 requested a review from vividf April 3, 2024 06:34
@knzo25
Copy link
Collaborator Author

knzo25 commented Apr 3, 2024

@vividf
Ok, we only need to fix the internal issues with the SfM calibrator and we are done 💪

vividf
vividf previously approved these changes Apr 3, 2024
@knzo25
Copy link
Collaborator Author

knzo25 commented Apr 3, 2024

Reminder to everyone seeing this to NOT merge until we finish the internal checks

@vividf vividf self-requested a review April 4, 2024 06:54
@vividf vividf dismissed their stale review April 4, 2024 06:54

looking into sfm again

Copy link
Contributor

@vividf vividf left a comment

Choose a reason for hiding this comment

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

@knzo25
While looking into the code in the manager, found a small naming problem.

Signed-off-by: Kenzo Lobos-Tsunekawa <kenzo.lobos@tier4.jp>
@knzo25 knzo25 requested a review from vividf April 9, 2024 05:48
vividf

This comment was marked as off-topic.

Copy link
Contributor

@vividf vividf left a comment

Choose a reason for hiding this comment

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

LGTM!

@knzo25 knzo25 merged commit a8a4b6a into tier4:tier4/universe Apr 11, 2024
5 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants