refactor(radar_tracks_msgs_converter): rework parameters #4911

PhoebeWu21 · 2023-09-07T06:23:09Z

Description

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

Remove the default value from the source code in order to ensure all parameter values are passed from the parameter files.
Create the schema

Tests performed

Package built and launch locally.
colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release --packages-up-to radar_tracks_msgs_converter

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.

I've confirmed the contribution guidelines.
The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

The PR follows the pull request guidelines.
The PR has been properly tested.
The PR has been reviewed by the code owners.

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.
The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

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

kaspermeck-arm

Looks good, but need more input.

perception/radar_tracks_msgs_converter/schema/radar_tracks_msgs_converter.schema.json

perception/radar_tracks_msgs_converter/config/config-radar_tracks_msgs_converter.param.yaml

...cks_msgs_converter/src/radar_tracks_msgs_converter_node/radar_tracks_msgs_converter_node.cpp

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

…rter

codecov · 2023-11-29T23:36:34Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (765a596) 15.32% compared to head (ac11ee3) 0.00%.
Report is 574 commits behind head on main.

Files	Patch %	Lines
...onverter_node/radar_tracks_msgs_converter_node.cpp	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #4911       +/-   ##
==========================================
- Coverage   15.32%   0.00%   -15.33%     
==========================================
  Files        1721       2     -1719     
  Lines      118559     138   -118421     
  Branches    37995       0    -37995     
==========================================
- Hits        18169       0    -18169     
+ Misses      79657     138    -79519     
+ Partials    20733       0    -20733

Flag	Coverage Δ
differential	`0.00% <0.00%> (?)`
total	`?`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

perception/radar_tracks_msgs_converter/config/config-radar_tracks_msgs_converter.param.yaml

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

…rter

scepter914 · 2023-12-05T09:35:07Z

perception/radar_tracks_msgs_converter/README.md

-  - Default parameter is "base_link"
- `use_twist_compensation` (bool): If the parameter is true, then the twist of output objects' topic is compensated by ego vehicle motion.
-  - Default parameter is "false"
+{{ json_to_markdown("perception/radar_tracks_msgs_converter/schema/radar_tracks_msgs_converter.schema.json") }}


[MUST]
This change is very inconvenience because we cannot see parameters description at github.
I think the parameter description should be written by markdown like this PR.

(Sorry for confusing with my comment above 🙇 )

@scepter914 This is an implementation of the goal described in https://autowarefoundation.github.io/autoware-documentation/main/contributing/coding-guidelines/ros-nodes/parameters/#workflow. Could you consider using the rendered version of the documentation instead of the github pages? For example: https://autowarefoundation.github.io/autoware.universe/latest/perception/lidar_apollo_segmentation_tvm_nodes/#parameters

Why we use only the rendered version of documents?
I think it is convenience both github README and the rendered version.

In addition, as the reaction in PR of #5241 is neglected for now, I cannot recognize the maintenance ability for using the json schema for parameter tables.
As code owner, I cannot accept the change that cause inconvenience for maintenance, and it is not allowed to delete the markdown parameter tables unless there is enough ability to maintain the json schema for parameter tables.

stale · 2024-02-06T03:35:33Z

This pull request has been automatically marked as stale because it has not had recent activity.

scepter914 · 2024-02-06T09:44:47Z

Because this PR has stale label and there are conflicts from many changes.
I close this PR once and ask to make new PR from now main branch.
Thank you for contributing.

kaspermeck-arm · 2024-02-21T21:47:18Z

@mitsudome-r @scepter914

I don't see any issue with this PR. Would it be possible to re-open this issue and work on getting it merged? Thanks!

scepter914 · 2024-02-23T09:37:53Z

@kaspermeck-arm
Many PR are merged from the time when this PR made and there are conflicts.
Would you solving this comment and make new PR from now main branch?

kaspermeck-arm · 2024-02-23T18:55:37Z

@kaspermeck-arm Many PR are merged from the time when this PR made and there are conflicts. Would you solving this comment and make new PR from now main branch?

If I'm understanding correctly, there are two issues:

Since this PR was created, new code has been merged to the radar_tracks_msgs_converter which requires updating this PR
Unwillingness to adopt the new code contributions guidelines for parameters
- E.g., single source of truth for documentation using the schema web documentation instead of README

@scepter914 - is this correct?

@mitsudome-r @ambroise-arm

scepter914 · 2024-02-23T20:35:46Z

Since this PR was created, new code has been merged to the radar_tracks_msgs_converter which requires updating this PR

Yes.
I cannot merge this PR until the changes match the current main branch code.

Unwillingness to adopt the new code contributions guidelines for parameters

Because of many documentation update the parameter table is not enough to explain the parameters as you can see radar_object_clustering.
So the replace from parameter documents of current version to the table made from json file like this PR cannot accept to merge.

As the parameter table change, I think the PR should not replace the parameter explain but only add tables.

Anyway, once this PR marked stale and there was a difference from the main branch as a result of being abandoned, so I think you should recreate the PR with a clean commit log as OSS develop.

kaspermeck-arm · 2024-02-24T00:18:19Z

Because of many documentation update the parameter table is not enough to explain the parameters as you can see radar_object_clustering. So the replace from parameter documents of current version to the table made from json file like this PR cannot accept to merge.

As the parameter table change, I think the PR should not replace the parameter explain but only add tables.

This is the section which should be replaced with the auto-generated schema-to-table macro with radar_tracks_msgs_converter.schema.json (once it's been updated) as source.

It was decided that having a single source of truth brings more value than having the parameter table in the README file. If you want to add more context outside of the README if the description field in the table isn't sufficient, like you express you want.

You can compare

https://autowarefoundation.github.io/autoware.universe/main/perception/radar_tracks_msgs_converter/#parameters

and

https://autowarefoundation.github.io/autoware.universe/main/perception/radar_object_tracker/#node-parameters

to see how it will look with the new coding guidelines. Looks pretty nice, yeah? :)

Anyway, once this PR marked stale and there was a difference from the main branch as a result of being abandoned, so I think you should recreate the PR with a clean commit log as OSS develop.

Agreed, a new PR based on this PR can be created instead of trying to fix this one.

@scepter914 - can we agree on the suggestions above?

scepter914 · 2024-02-24T18:10:12Z

I cannot agree your suggestion.
The reason radar_object_tracker looks well for you is the few amount of explanation.
If we add many explanation like as radar_object_clustering, the table parameter is not suitable.
Because we will add documentations for Autoware packages including this package day by day, I don't think "replacing to parameter table" is a good idea at all.

In summary, I cannot accept the change "replacing from raw README to parameter table made from a json file" and I can only accept the change "adding parameter table made from a json file with a short description" like below table.

Name	Type	Description	Default value
`angle_threshold`	double	Angle threshold to cluster objects	0.174
`distance_threshold`	double	Distance threshold to cluster objects	4.0
`velocity_threshold`	double	Velocity threshold to cluster objects	2.0
`is_fixed_label`	bool	The flag to use fixed label	false
`fixed_label`	string	The label of a clustered object	"UNKNOWN"
`is_fixed_size`	bool	The flag to use fixed size	false
`size_x`	double	The x-axis size of object	4.0
`size_y`	double	The y-axis size of object	1.5
`size_z`	double	The z-axis size of object	1.5

refactor(radar_tracks_msgs_converter): rework parameters

2ce48f3

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

PhoebeWu21 requested review from scepter914 and miursh as code owners September 7, 2023 06:23

PhoebeWu21 requested a review from ambroise-arm September 7, 2023 06:23

github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Sep 7, 2023

PhoebeWu21 requested a review from kaspermeck-arm September 7, 2023 06:23

style(pre-commit): autofix

ed10909

ambroise-arm requested a review from mitsudome-r September 13, 2023 15:29

kaspermeck-arm reviewed Sep 19, 2023

View reviewed changes

perception/radar_tracks_msgs_converter/schema/radar_tracks_msgs_converter.schema.json Show resolved Hide resolved

scepter914 requested changes Oct 16, 2023

View reviewed changes

perception/radar_tracks_msgs_converter/config/config-radar_tracks_msgs_converter.param.yaml Outdated Show resolved Hide resolved

...cks_msgs_converter/src/radar_tracks_msgs_converter_node/radar_tracks_msgs_converter_node.cpp Outdated Show resolved Hide resolved

PhoebeWu21 and others added 3 commits October 16, 2023 13:39

refactor(radar_tracks_msgs_converter): rework parameters

218cb31

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

Merge branch 'main' into refactor-node-config-radar_tracks_msgs_conve…

1339da7

…rter

Merge branch 'main' into refactor-node-config-radar_tracks_msgs_conve…

285ed26

…rter

ambroise-arm approved these changes Nov 27, 2023

View reviewed changes

kminoda added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Nov 29, 2023

kminoda reviewed Dec 1, 2023

View reviewed changes

perception/radar_tracks_msgs_converter/config/config-radar_tracks_msgs_converter.param.yaml Show resolved Hide resolved

refactor(radar_tracks_msgs_converter): rework parameters

66b173c

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

github-actions bot added the type:documentation Creating or refining documentation. (auto-assigned) label Dec 4, 2023

Merge branch 'main' into refactor-node-config-radar_tracks_msgs_conve…

ac11ee3

…rter

scepter914 requested changes Dec 5, 2023

View reviewed changes

stale bot added the status:stale Inactive or outdated issues. (auto-assigned) label Feb 6, 2024

scepter914 closed this Feb 6, 2024

PhoebeWu21 deleted the refactor-node-config-radar_tracks_msgs_converter branch February 7, 2024 00:37

kaspermeck-arm mentioned this pull request Mar 2, 2024

refactor(radar_tracks_msgs_converter): rework parameters #6533

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(radar_tracks_msgs_converter): rework parameters #4911

refactor(radar_tracks_msgs_converter): rework parameters #4911

PhoebeWu21 commented Sep 7, 2023 •

edited

Loading

kaspermeck-arm left a comment

codecov bot commented Nov 29, 2023 •

edited

Loading

scepter914 Dec 5, 2023

kminoda Dec 5, 2023

ambroise-arm Dec 5, 2023

scepter914 Dec 8, 2023 •

edited

Loading

stale bot commented Feb 6, 2024

scepter914 commented Feb 6, 2024

kaspermeck-arm commented Feb 21, 2024

scepter914 commented Feb 23, 2024 •

edited

Loading

kaspermeck-arm commented Feb 23, 2024

scepter914 commented Feb 23, 2024

kaspermeck-arm commented Feb 24, 2024 •

edited

Loading

scepter914 commented Feb 24, 2024 •

edited

Loading

refactor(radar_tracks_msgs_converter): rework parameters #4911

refactor(radar_tracks_msgs_converter): rework parameters #4911

Conversation

PhoebeWu21 commented Sep 7, 2023 • edited Loading

Description

Tests performed

Effects on system behavior

Pre-review checklist for the PR author

In-review checklist for the PR reviewers

Post-review checklist for the PR author

kaspermeck-arm left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 29, 2023 • edited Loading

Codecov Report

scepter914 Dec 5, 2023

Choose a reason for hiding this comment

kminoda Dec 5, 2023

Choose a reason for hiding this comment

ambroise-arm Dec 5, 2023

Choose a reason for hiding this comment

scepter914 Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

stale bot commented Feb 6, 2024

scepter914 commented Feb 6, 2024

kaspermeck-arm commented Feb 21, 2024

scepter914 commented Feb 23, 2024 • edited Loading

kaspermeck-arm commented Feb 23, 2024

scepter914 commented Feb 23, 2024

kaspermeck-arm commented Feb 24, 2024 • edited Loading

scepter914 commented Feb 24, 2024 • edited Loading

PhoebeWu21 commented Sep 7, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading

scepter914 Dec 8, 2023 •

edited

Loading

scepter914 commented Feb 23, 2024 •

edited

Loading

kaspermeck-arm commented Feb 24, 2024 •

edited

Loading

scepter914 commented Feb 24, 2024 •

edited

Loading