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

refactor(autoware_pointcloud_preprocessor): rework ring outlier filter parameters #8468

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

vividf
Copy link
Contributor

@vividf vividf commented Aug 14, 2024

Description

Description

This PR includes the following changes

  1. Remove the default parameters for the ring outlier filter node.
  2. Add parameter, launch file, and parameters schema for the node.
  3. Change the name nodelet to node.

A big thank you to @Ariiees for the PRs (#8297 and #7422), where we worked together on the entire pointcloud preprocessor parameters, launch file, and schema.

Note that the schema for filter.param.yaml will be implemented in another PR.

Related links

Parent Issue:

  • Link

How was this PR tested?

ros2 launch autoware_pointcloud_preprocessor ring_outlier_filter_node.launch.xml

Notes for reviewers

Need to merge together with
tier4/aip_launcher#305
autowarefoundation/sample_sensor_kit_launch#103

Interface changes

None.

Effects on system behavior

None.

Signed-off-by: vividf <yihsiang.fang@tier4.jp>
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) labels Aug 14, 2024
Copy link

github-actions bot commented Aug 14, 2024

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

@vividf vividf self-assigned this Aug 14, 2024
@vividf vividf marked this pull request as ready for review August 14, 2024 15:45
@vividf vividf added tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) labels Aug 14, 2024
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

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

Project coverage is 24.16%. Comparing base (1bf24e4) to head (1a1ecf9).

Files with missing lines Patch % Lines
...or/src/outlier_filter/ring_outlier_filter_node.cpp 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8468      +/-   ##
==========================================
- Coverage   27.98%   24.16%   -3.83%     
==========================================
  Files        1319     1416      +97     
  Lines       98732   103181    +4449     
  Branches    39794    39518     -276     
==========================================
- Hits        27631    24930    -2701     
- Misses      71052    75868    +4816     
- Partials       49     2383    +2334     
Flag Coverage Δ *Carryforward flag
differential 21.58% <0.00%> (?)
total 24.12% <ø> (-3.87%) ⬇️ Carriedforward from cd0be8e

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

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

Copy link
Contributor

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

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

Left some comments (please check those that apply to the other PRs in this series of parameter related PRs)

Signed-off-by: vividf <yihsiang.fang@tier4.jp>
Signed-off-by: vividf <yihsiang.fang@tier4.jp>
Signed-off-by: vividf <yihsiang.fang@tier4.jp>
@vividf vividf requested a review from knzo25 August 20, 2024 09:59
@vividf
Copy link
Contributor Author

vividf commented Aug 27, 2024

Note: need to merge together with the PR autowarefoundation/sample_sensor_kit_launch#103

vividf and others added 2 commits August 28, 2024 16:26
Signed-off-by: vividf <yihsiang.fang@tier4.jp>
@badai-nguyen
Copy link
Contributor

@vividf Could you also update the parameters for RingOutlierFilterComponent in aip_launcher

@vividf
Copy link
Contributor Author

vividf commented Aug 30, 2024

@badai-nguyen Sure, I will update it as well.

@knzo25
Copy link
Contributor

knzo25 commented Sep 6, 2024

@vividf
Remember to paste links of related PRs (like the one dai-san asked you). It is important to keep track as just pasting the link adds an event in the timeline of both PRs

@vividf
Copy link
Contributor Author

vividf commented Sep 10, 2024

@vividf vividf requested a review from knzo25 September 10, 2024 01:43
"description": "The right limit of azimuth for visibility score calculation",
"default": "360.0",
"minimum": 0,
"maximum": 360
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not how schema handles this, but ROS2 is quite squeamish about how it parses ints and floats differently.

-> 360.0

Also, some numbers are between commas and other not. Which is the correct version? Make it consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@knzo25 Thanks for the review. For the default value, from Amadeusz's suggestion, assigning the default values as strings will pass the number precisely.

Minimum and maximum values do not accept commas. Also, from the website examples and by testing local, 360 and 360.0 are the same (as they are just checking for the boundary but not related to ros2 parsing?)

Copy link
Contributor

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

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

See the previous comment. Just checked and this applies in several places of this PR. Check other PRs and also the ones of this series that you already merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) tag:require-cuda-build-and-test tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants