-
Notifications
You must be signed in to change notification settings - Fork 640
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
refactor(radar_fusion_to_detected_object): rework parameters #4663
Conversation
Signed-off-by: PhoebeWu21 <[email protected]>
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.
LGTM
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.
Looks good!
core_param_.threshold_probability = | ||
declare_parameter<float>("core_params.threshold_probability", 0.0); |
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 actually double
. So for clarity we should write declare_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.
- Debug it in this PR.
- Once revert the changing the parameter type and merge this PR for main branch, and make new PR for the commit of 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.
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?
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #4663 +/- ##
=======================================
Coverage 15.12% 15.12%
=======================================
Files 1571 1571
Lines 108229 108165 -64
Branches 33224 33195 -29
=======================================
- Hits 16367 16365 -2
+ Misses 74032 73971 -61
+ Partials 17830 17829 -1
*This pull request uses carry forward flags. Click here to find out more.
☔ View full report in Codecov by Sentry. |
…efoundation#4663) * refactor(radar_fusion_to_detected_object): rework parameters Signed-off-by: PhoebeWu21 <[email protected]> * style(pre-commit): autofix * style(pre-commit): autofix * style(pre-commit): autofix --------- Signed-off-by: PhoebeWu21 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Satoshi Tanaka <[email protected]>
…efoundation#4663) * refactor(radar_fusion_to_detected_object): rework parameters Signed-off-by: PhoebeWu21 <[email protected]> * style(pre-commit): autofix * style(pre-commit): autofix * style(pre-commit): autofix --------- Signed-off-by: PhoebeWu21 <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Satoshi Tanaka <[email protected]>
Description
Implement the ROS Node configuration layout described in https://github.com/orgs/autowarefoundation/discussions/3371 for the radar_fusion_to_detected_object package.
Tests performed
Package built locally.
colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release --packages-up-to radar_fusion_to_detected_object
Effects on system behavior
More reliable and faster parameter configuration file creation.
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.
After all checkboxes are checked, anyone who has write access can merge the PR.