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(ndt): remove ndt package #2053

Merged
merged 18 commits into from
Oct 27, 2022

Conversation

kminoda
Copy link
Contributor

@kminoda kminoda commented Oct 12, 2022

Description

Remove ndt package.

The package was intended to provide a unique interface for various types of NDT implementation (like ndt_omp or pcl ones), but in real current Autoware is designed to use ndt_omp implementation. (e.g. the parameters are only designed for ndt_omp). In addition, as far as we know, NDT implementations other than ndt_omp does not seem to be used.

Thus, I propose to remove ndt and ndt_pcl_modified packages to simplify the localization module and especially ndt_scan_matcher.

Related links

[proposal] Removing NDT implementations other than ndt_omp

Tests performed

I have ran Autoware with the sample data from tutorial, and confirmed that the localization module works fine.

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.

kminoda and others added 4 commits October 12, 2022 11:38
Signed-off-by: kminoda <koji.m.minoda@gmail.com>
Signed-off-by: kminoda <koji.minoda@tier4.jp>
Signed-off-by: kminoda <koji.minoda@tier4.jp>
Signed-off-by: kminoda <koji.minoda@tier4.jp>
@kminoda kminoda self-assigned this Oct 12, 2022
@kminoda kminoda added the component:localization Vehicle's position determination in its environment. (auto-assigned) label Oct 12, 2022
kminoda and others added 2 commits October 12, 2022 17:42
Signed-off-by: kminoda <koji.minoda@tier4.jp>
@kminoda kminoda marked this pull request as draft October 12, 2022 09:01
@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Base: 10.77% // Head: 10.75% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (7fe5ddf) compared to base (893a3bb).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2053      +/-   ##
==========================================
- Coverage   10.77%   10.75%   -0.03%     
==========================================
  Files        1186     1181       -5     
  Lines       84843    84723     -120     
  Branches    19898    19819      -79     
==========================================
- Hits         9144     9108      -36     
+ Misses      66007    65995      -12     
+ Partials     9692     9620      -72     
Flag Coverage Δ *Carryforward flag
differential 0.00% <0.00%> (?)
total 10.75% <0.00%> (-0.01%) ⬇️ Carriedforward from 8fb13c9

*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%> (ø)
...ion/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp 0.00% <0.00%> (ø)
...ehicle/raw_vehicle_cmd_converter/src/brake_map.cpp 33.33% <0.00%> (-11.83%) ⬇️
..._converter/test/test_raw_vehicle_cmd_converter.cpp 27.77% <0.00%> (-4.23%) ⬇️
...planning_evaluator/src/planning_evaluator_node.cpp 37.11% <0.00%> (-1.04%) ⬇️
planning/behavior_path_planner/src/utilities.cpp 3.07% <0.00%> (-0.77%) ⬇️
control/trajectory_follower/src/mpc.cpp 48.13% <0.00%> (-0.40%) ⬇️
...ctory_follower/src/pid_longitudinal_controller.cpp 37.11% <0.00%> (-0.26%) ⬇️
perception/shape_estimation/src/node.cpp 0.00% <0.00%> (ø)
sensing/gnss_poser/src/gnss_poser_core.cpp 0.00% <0.00%> (ø)
... and 72 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.

@kminoda kminoda marked this pull request as ready for review October 19, 2022 04:34
@KYabuuchi
Copy link
Contributor

Thank you for great PR!!
I built and tested your branch, and the NDT worked successfully.
I'll check out the details soon.

kminoda and others added 3 commits October 20, 2022 17:32
Signed-off-by: kminoda <koji.minoda@tier4.jp>
Signed-off-by: kminoda <koji.minoda@tier4.jp>
@KYabuuchi
Copy link
Contributor

  • There is still a description about OMP in ndt_scan_matcher/config/ndt_scan_matcher.param.yaml#L25.
    image

  • There is no default value for converged_param_type in tier4_localization_launch/config/ndt_scan_matcher.param.yaml.
    (On the other hand, ndt_scan_matcher/config/ndt_scan_matcher.param.yaml has the default value)

  • There is no description of converged_type in the ndt_scan_matcher/README.md.
    If you don't mind, could you add something like following?

Type of termination criteria for iterative calculations (0=TRANSFORM_PROBABILITY, 1=NEAREST_VOXEL_TRANSFORMATION_LIKELIHOOD)"

Signed-off-by: kminoda <koji.minoda@tier4.jp>
…ware.universe into refactor/ndt/remove_package
Signed-off-by: kminoda <koji.minoda@tier4.jp>
Copy link
Contributor

@KYabuuchi KYabuuchi left a comment

Choose a reason for hiding this comment

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

LGTM

@KYabuuchi
Copy link
Contributor

@YamatoAndo I am not the owner on this branch and cannot give final approval.
Could you please review this PR in your free time?

(In the main branch I am the owner of ndt_scan_matcher , but this PR is derived from before that.)

@kminoda
Copy link
Contributor Author

kminoda commented Oct 27, 2022

I think it depends on this one
#2118

@kminoda
Copy link
Contributor Author

kminoda commented Oct 27, 2022

Oh, I think this PR is blocked since @.YamatoAndo is the only code owner of ndt package (which will be removed with this PR).

@YamatoAndo Would you also approve this PR?

Signed-off-by: kminoda <koji.minoda@tier4.jp>
kminoda and others added 3 commits October 27, 2022 16:24
Signed-off-by: kminoda <koji.minoda@tier4.jp>
…ware.universe into refactor/ndt/remove_package
@kminoda kminoda merged commit 3eb3b64 into autowarefoundation:main Oct 27, 2022
@kminoda kminoda deleted the refactor/ndt/remove_package branch November 30, 2022 00:13
HansRobo pushed a commit to HansRobo/autoware.universe that referenced this pull request Dec 16, 2022
* first commit

Signed-off-by: kminoda <koji.m.minoda@gmail.com>

* CMakeLists.txt does not work........

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* build works

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* debugged

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* remove unnecessary parameter

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* ci(pre-commit): autofix

* removed 'omp'-related words completely

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* ci(pre-commit): autofix

* fixed param description of converged_param

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* remove OMPParams

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* removed unnecessary includes

Signed-off-by: kminoda <koji.minoda@tier4.jp>

* removed default parameter from search_method

* small fix

Signed-off-by: kminoda <koji.minoda@tier4.jp>

Signed-off-by: kminoda <koji.m.minoda@gmail.com>
Signed-off-by: kminoda <koji.minoda@tier4.jp>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: Kotaro Yoshimoto <pythagora.yoshimoto@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:localization Vehicle's position determination in its environment. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants