-
Notifications
You must be signed in to change notification settings - Fork 639
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(radar_fusion_to_detected_object): rework parameters #4663
Merged
scepter914
merged 9 commits into
autowarefoundation:main
from
PhoebeWu21:refactor-node-config-radar_fusion_to_detected_object
Aug 30, 2023
Merged
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
b0aba4c
refactor(radar_fusion_to_detected_object): rework parameters
PhoebeWu21 0665c2c
style(pre-commit): autofix
pre-commit-ci[bot] de52eb4
Merge branch 'main' into refactor-node-config-radar_fusion_to_detecte…
scepter914 9e17990
style(pre-commit): autofix
pre-commit-ci[bot] 82ee585
Merge branch 'main' into refactor-node-config-radar_fusion_to_detecte…
scepter914 11636b8
style(pre-commit): autofix
pre-commit-ci[bot] e20eb0f
Merge branch 'main' into refactor-node-config-radar_fusion_to_detecte…
scepter914 fe7a6c4
Merge branch 'main' into refactor-node-config-radar_fusion_to_detecte…
scepter914 2a3382e
Merge branch 'main' into refactor-node-config-radar_fusion_to_detecte…
scepter914 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
114 changes: 114 additions & 0 deletions
114
...eption/radar_fusion_to_detected_object/schema/radar_fusion_to_detected_object.schema.json
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
{ | ||
"$schema": "http://json-schema.org/draft-07/schema#", | ||
"title": "Parameters for Radar Fusion to Detected Object Node", | ||
"type": "object", | ||
"definitions": { | ||
"radar_fusion_to_detected_object": { | ||
"type": "object", | ||
"properties": { | ||
"node_params": { | ||
"type": "object", | ||
"properties": { | ||
"update_rate_hz": { | ||
"type": "number", | ||
"description": "Update rate for parameters. [hz]", | ||
"default": "10.0", | ||
"minimum": 0.0 | ||
} | ||
}, | ||
"required": ["update_rate_hz"] | ||
}, | ||
"core_params": { | ||
"type": "object", | ||
"properties": { | ||
"bounding_box_margin": { | ||
"type": "number", | ||
"description": "The distance to extend the 2D bird's-eye view Bounding Box on each side. This distance is used as a threshold to find radar centroids falling inside the extended box. [m]", | ||
"default": "2.0", | ||
"minimum": 0.0 | ||
}, | ||
"split_threshold_velocity": { | ||
"type": "number", | ||
"description": "The object's velocity threshold to decide to split for two objects from radar information (currently not implemented) [m/s]", | ||
"default": "5.0", | ||
"minimum": 0.0 | ||
}, | ||
"threshold_yaw_diff": { | ||
"type": "number", | ||
"description": "The yaw orientation threshold. If ∣θob−θra∣<threshold×yaw_diff attached to radar information include estimated velocity, where `θob` is yaw angle from 3d detected object, `θra` is yaw angle from radar object. [rad]", | ||
"default": "0.35" | ||
}, | ||
"velocity_weight_min_distance": { | ||
"type": "number", | ||
"description": "The twist coefficient of radar data nearest to the center of bounding box in velocity estimation.", | ||
"default": "1.0" | ||
}, | ||
"velocity_weight_average": { | ||
"type": "number", | ||
"description": "The twist coefficient of average twist of radar data in velocity estimation.", | ||
"default": "0.0" | ||
}, | ||
"velocity_weight_median": { | ||
"type": "number", | ||
"description": "The twist coefficient of median twist of radar data in velocity estimation.", | ||
"default": "0.0" | ||
}, | ||
"velocity_weight_target_value_average": { | ||
"type": "number", | ||
"description": "The twist coefficient of target value weighted average in velocity estimation. Target value is amplitude if using radar pointcloud. Target value is probability if using radar objects.", | ||
"default": "0.0" | ||
}, | ||
"velocity_weight_target_value_top": { | ||
"type": "number", | ||
"description": "The twist coefficient of top target value radar data in velocity estimation. Target value is amplitude if using radar pointcloud. Target value is probability if using radar objects.", | ||
"default": "0.0" | ||
}, | ||
"convert_doppler_to_twist": { | ||
"type": "boolean", | ||
"description": "Convert doppler velocity to twist using the yaw information of a detected object.", | ||
"default": "false" | ||
}, | ||
"threshold_probability": { | ||
"type": "number", | ||
"description": "If the probability of an output object is lower than this parameter, and the output object does not have radar points/objects, then delete the object.", | ||
"default": "0.4", | ||
"minimum": 0.0, | ||
"maximum": 1.0 | ||
}, | ||
"compensate_probability": { | ||
"type": "boolean", | ||
"description": "If this parameter is true, compensate probability of objects to threshold probability.", | ||
"default": "false" | ||
} | ||
}, | ||
"required": [ | ||
"bounding_box_margin", | ||
"split_threshold_velocity", | ||
"threshold_yaw_diff", | ||
"velocity_weight_min_distance", | ||
"velocity_weight_average", | ||
"velocity_weight_median", | ||
"velocity_weight_target_value_average", | ||
"velocity_weight_target_value_top", | ||
"convert_doppler_to_twist", | ||
"threshold_probability", | ||
"compensate_probability" | ||
] | ||
} | ||
}, | ||
"required": ["node_params", "core_params"] | ||
} | ||
}, | ||
"properties": { | ||
"/**": { | ||
"type": "object", | ||
"properties": { | ||
"ros__parameters": { | ||
"$ref": "#/definitions/radar_fusion_to_detected_object" | ||
} | ||
}, | ||
"required": ["ros__parameters"] | ||
} | ||
}, | ||
"required": ["/**"] | ||
} |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[MUST] Probability value is defined by float in Autoware message. Please revert to float.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @scepter914 , it was "float" in my previous commit, but @ambroise-arm asked me to change to "double" because of the new Declare Parameter Function. ( see in #4537 (comment))
Please let me know which to follow. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scepter914 Yes, as I wrote in the comment linked above, the return type of
declare_parameter<float>()
is actuallydouble
. So for clarity we should writedeclare_parameter<double>()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PhoebeWu21 @ambroise-arm
Thank you for comment.
I agree with the reason changing from "float" to "double", but pre-commit.ci failed.
So I cannot approve until you debug this build fail.
I think two way to solve this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how the change from float to double relates to the pre-commit.ci failure. It is not a build job but a linting job, and the failures it reports look unrelated to the changes in this PR.
Let's see what the
build-and-test*
jobs result in for building the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no pre-commit.ci failures in other PR. So I guess build-and-test* jobs don't have problem and your change cause pre-commit.ci failures.
I recommend that you construct local environment and debug it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @scepter914,
Finally, pre-commit.ci check has passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PhoebeWu21
[imo] Since
core_param_.threshold_probability
is defined as float, this is implicit type conversion here.Is this meant?