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(perception/occupancy_grid_map_outlier_filter): rework parameters #6745

Merged
merged 5 commits into from
Aug 30, 2024

Conversation

oguzkaganozt
Copy link
Contributor

@oguzkaganozt oguzkaganozt commented Apr 4, 2024

Description

Implement the ROS Node configuration layout described in https://github.com/orgs/autowarefoundation/discussions/3371

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

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.

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.

@oguzkaganozt oguzkaganozt added type:documentation Creating or refining documentation. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) labels Apr 4, 2024
@oguzkaganozt oguzkaganozt self-assigned this Apr 4, 2024
@YoshiRi
Copy link
Contributor

YoshiRi commented Jun 3, 2024

Sorry for super-late reply. 🙇
Though the changes LGTM within this package, this PR breaks current sample execution due to launch file is not well defined in the user of this outlier filter.
I will convert it to draft for now. Re-open this PR after sample rosbag execution problem is fixed.

@YoshiRi YoshiRi marked this pull request as draft June 3, 2024 16:32
@oguzkaganozt
Copy link
Contributor Author

Any updates from rosbag execution problem ? @YoshiRi

@oguzkaganozt oguzkaganozt force-pushed the refactor/occupancy_grid_map_outlier_filter branch 2 times, most recently from cde7a94 to 1fcd053 Compare August 12, 2024 16:41
Copy link

github-actions bot commented Aug 12, 2024

Thank you for contributing to the Autoware project!

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

Please ensure:

@oguzkaganozt oguzkaganozt marked this pull request as ready for review August 12, 2024 16:41
@oguzkaganozt
Copy link
Contributor Author

Ready for review @YoshiRi

@YoshiRi
Copy link
Contributor

YoshiRi commented Aug 23, 2024

@oguzkaganozt cc: @Shin-kyoto @badai-nguyen
Sorry about my super late reply 🙇

This package change seems to be okay, but this will cause trouble with occupancy_grid_based_outlier_filter node declaration in ground_segmentation.launch.py.

I think we need following approach:

    1. add your config file to autoware_launch
    1. modify ground_segmentation.launch.py to look for the config file

I will do 1. first, and want to merge this PR after 2. is solved. ( to prevent autoware crush )

@YoshiRi
Copy link
Contributor

YoshiRi commented Aug 23, 2024

@badai-nguyen
Could you review this?

@oguzkaganozt oguzkaganozt force-pushed the refactor/occupancy_grid_map_outlier_filter branch from 5253385 to e1a91fe Compare August 26, 2024 13:03
@oguzkaganozt
Copy link
Contributor Author

Hi any updates on this ? @YoshiRi @badai-nguyen

@oguzkaganozt
Copy link
Contributor Author

Hi any updates on this ? @badai-nguyen @YoshiRi

@oguzkaganozt oguzkaganozt enabled auto-merge (squash) August 26, 2024 13:19
Copy link

codecov bot commented Aug 26, 2024

Codecov Report

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

Project coverage is 24.15%. Comparing base (eac6332) to head (70195b3).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...ter/src/occupancy_grid_map_outlier_filter_node.cpp 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6745      +/-   ##
==========================================
- Coverage   24.15%   24.15%   -0.01%     
==========================================
  Files        1400     1400              
  Lines      102252   102261       +9     
  Branches    38758    38761       +3     
==========================================
  Hits        24697    24697              
- Misses      75046    75055       +9     
  Partials     2509     2509              
Flag Coverage Δ *Carryforward flag
differential 0.00% <0.00%> (?)
total 24.15% <ø> (+<0.01%) ⬆️ Carriedforward from eac6332

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

@oguzkaganozt oguzkaganozt force-pushed the refactor/occupancy_grid_map_outlier_filter branch from f2bf929 to aac3fa7 Compare August 27, 2024 21:47
@YoshiRi
Copy link
Contributor

YoshiRi commented Aug 29, 2024

I think we need to change related launcher:
YoshiRi@80bfc95

But sorry, I had not tested yet.
@oguzkaganozt
If you do not mind, can anyone test with loggin simulator?

autoware_launch need to be updated before testing this.

Copy link
Contributor

@badai-nguyen badai-nguyen left a comment

Choose a reason for hiding this comment

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

This package change seems to be okay, but this will cause trouble with occupancy_grid_based_outlier_filter node declaration in ground_segmentation.launch.py.
I think we need following approach:

    1. add your config file to autoware_launch
    1. modify ground_segmentation.launch.py to look for the config file
      I will do 1. first, and want to merge this PR after 2. is solved. ( to prevent autoware crush )

@oguzkaganozt Thank you for your contribution,
I think @YoshiRi already left a comment as above, could you confirm and solve 2 as his suggestion?

@github-actions github-actions bot added the component:launch Launch files, scripts and initialization tools. (auto-assigned) label Aug 30, 2024
Copy link
Contributor

@YoshiRi YoshiRi left a comment

Choose a reason for hiding this comment

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

I fixed launch problem in a6f83ef and f3d008d.
Tested with my local PC.

@Shin-kyoto @badai-nguyen
Could you update the status?

@oguzkaganozt oguzkaganozt force-pushed the refactor/occupancy_grid_map_outlier_filter branch from 0d13734 to 70195b3 Compare August 30, 2024 09:59
@YoshiRi YoshiRi dismissed Shin-kyoto’s stale review August 30, 2024 17:00

It already resolved

@oguzkaganozt oguzkaganozt merged commit 3e4a945 into main Aug 30, 2024
31 checks passed
@oguzkaganozt oguzkaganozt deleted the refactor/occupancy_grid_map_outlier_filter branch August 30, 2024 17:00
ktro2828 pushed a commit to ktro2828/autoware.universe that referenced this pull request Sep 18, 2024
…ters (autowarefoundation#6745)

* add param and schema file, edit readme

Signed-off-by: oguzkaganozt <[email protected]>

* .

Signed-off-by: Oguz Ozturk <[email protected]>

* correct linter errors

Signed-off-by: oguzkaganozt <[email protected]>

---------

Signed-off-by: oguzkaganozt <[email protected]>
Signed-off-by: Oguz Ozturk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:launch Launch files, scripts and initialization tools. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (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
Development

Successfully merging this pull request may close these issues.

4 participants