feat(euclidean_cluster): add diagnostics warning when cluster skipped #10278
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
March 17, 2025 11:40
… due to excessive points from large objects
github-actions
bot
added
the
component:perception
|
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
ogrex
changed the title
technolojin
requested changes
Mar 18, 2025
| @@ -136,10 +168,19 @@ bool VoxelGridBasedEuclideanCluster::cluster( | |||
|
|
|||
| // build output and check cluster size | |||
| { | |||
| // At the start, create a container to collect warnings. | |||
| std::vector<std::string> warning_messages; | |||
There was a problem hiding this comment.
warning_messages could be diagnostics level not only WARN but ERROR.
Therefore I would like to suggest
Suggested change
| std::vector<std::string> warning_messages; | |
| std::vector<std::string> diagnostics_messages; |
|
|
||
| // After processing all clusters, publish a summary of diagnostics. | ||
| void VoxelGridBasedEuclideanCluster::publishDiagnosticsSummary( | ||
| const std::vector<std::string> & warnings, |
There was a problem hiding this comment.
Suggested change
| const std::vector<std::string> & warnings, | |
| const std::vector<std::string> & diagnostics_messages, |
There was a problem hiding this comment.
Here is two option
- If the cluster size is the only diagnostics and other diagnostics will not be added in the future, it shall be named
diagnostics_cluster_max_sizeor something similar. - There is any chance to add different type of diagnostics, pair with an index to distinguish diag type, and it will named
diagnostics_messages
Comment on lines
182
to
183
| warning_messages.push_back( | ||
| " Cluster " + std::to_string(i) + " (" + std::to_string(cluster_size) + ")."); |
There was a problem hiding this comment.
Suggested change
| warning_messages.push_back( | |
| " Cluster " + std::to_string(i) + " (" + std::to_string(cluster_size) + ")."); | |
| warning_messages.push_back( | |
| " Cluster " + std::to_string(i) + " (" + std::to_string(cluster_size) + " > "+ max_cluster_size_ +")."); |
There was a problem hiding this comment.
Indicate current maximum value to user.
There was a problem hiding this comment.
The diagnostics messages are modified in the latest commit as the following reasons
- current cluster implementation will make the point number in skipped cluster just exactly the same as threshold+1, making the information of point number in a cluster redundant
- without cluster point number, cluster index becomes redundant
so now output will only gives information of threshold and total number of skipped clusters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
add diagnostics warning when cluster skipped due to excessive points from large objects
Related links
Parent Issue:
How was this PR tested?
Tested by running logging simulator
OK:
Warnings:
Notes for reviewers
None.
Interface changes
None.
Effects on system behavior
None.