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

fix(euclidean_cluster): fix max_cluster_size bug #7734

Conversation

badai-nguyen
Copy link
Contributor

@badai-nguyen badai-nguyen commented Jun 27, 2024

Description

Related links

How was this PR tested?

  • Confirmed by logging_simulator and confirmed that a large cluster ( > max_cluster_size) will not be clustered.
  • Tested with max_cluster_size = 100
    image

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

@github-actions github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Jun 27, 2024
Copy link

github-actions bot commented Jun 27, 2024

Thank you for contributing to the Autoware project!

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

Please ensure:

@beginningfan
Copy link
Contributor

I think the problem is in line 120. When adding points to the cluster, the cluster stops adding when the size of cluster is equal to max_cluster_size. Therefore, this incomplete cluster will not be filtered out by line 134. According to your modification, the cluster will be filtered out by line 134 when it exits when there is one more point than max_cluster_size. However, this solution will cause the program to crash due to insufficient space allocated by resize in line 109, causing the program to crash. If the space is reallocated using the method I provided in #7662, or add an extra point when allocating space in line 109, this problem can be avoided.

@badai-nguyen
Copy link
Contributor Author

@beginningfan Yes, I created this PR by assuming your PR#7662 is merged. It is reason I still keep it as draft. Could you merge your PR? I'm sorry for insufficient description.

@badai-nguyen badai-nguyen marked this pull request as ready for review July 1, 2024 23:31
@badai-nguyen badai-nguyen added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Jul 2, 2024
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 28.39%. Comparing base (c2f9579) to head (c2488b3).
Report is 2 commits behind head on main.

Files Patch % Lines
...cluster/lib/voxel_grid_based_euclidean_cluster.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7734      +/-   ##
==========================================
+ Coverage   28.38%   28.39%   +0.01%     
==========================================
  Files        1584     1585       +1     
  Lines      115463   115486      +23     
  Branches    49233    49238       +5     
==========================================
+ Hits        32773    32794      +21     
- Misses      73684    73695      +11     
+ Partials     9006     8997       -9     
Flag Coverage Δ *Carryforward flag
differential 2.56% <0.00%> (?)
total 28.40% <ø> (+0.01%) ⬆️ Carriedforward from b5cbc44

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

@badai-nguyen
Copy link
Contributor Author

@beginningfan I updated this PR as your PR #7662 was merged. Since you also worked on Euclidean Cluster, could I ask your review on this PR again.

Copy link
Contributor

@beginningfan beginningfan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM

@badai-nguyen badai-nguyen merged commit 3763857 into autowarefoundation:main Jul 3, 2024
29 of 30 checks passed
badai-nguyen added a commit to tier4/autoware.universe that referenced this pull request Jul 3, 2024
badai-nguyen added a commit to tier4/autoware.universe that referenced this pull request Jul 4, 2024
…1378)

* fix(euclidean_cluster): fix euclidean cluster params (autowarefoundation#7662)

* fix(euclidean_cluster): fix max and min cluster size

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

* fix(gnss_poser): fix a typo

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

* fix(euclidean_cluster): fix min cluster size

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

* style(pre-commit): autofix

---------

Signed-off-by: beginningfan <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* fix(euclidean_cluster): fix max_cluster_size bug (autowarefoundation#7734)

Signed-off-by: badai-nguyen <[email protected]>

* fix(ground_segmentation): fix bug  (autowarefoundation#7771)

---------

Signed-off-by: beginningfan <[email protected]>
Signed-off-by: badai-nguyen <[email protected]>
Co-authored-by: beginningfan <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Shunsuke Miura <[email protected]>
palas21 pushed a commit to palas21/autoware.universe that referenced this pull request Jul 12, 2024
tby-udel pushed a commit to tby-udel/autoware.universe that referenced this pull request Jul 14, 2024
KhalilSelyan pushed a commit that referenced this pull request Jul 22, 2024
Ariiees pushed a commit to Ariiees/autoware.universe that referenced this pull request Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants