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

refactor(gnss_poser): rework parameters #4693

Merged

Conversation

PhoebeWu21
Copy link
Member

@PhoebeWu21 PhoebeWu21 commented Aug 22, 2023

Description

Implement the ROS Node configuration layout described in https://github.com/orgs/autowarefoundation/discussions/3371 for the gnss_poser 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 successfully build and launch locally.
colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release --packages-up-to gnss_poser

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.

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

@github-actions github-actions bot added the component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) label Aug 22, 2023
@YamatoAndo YamatoAndo added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Aug 22, 2023
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (017aed4) 15.32% compared to head (ce56b2b) 14.63%.
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4693      +/-   ##
==========================================
- Coverage   15.32%   14.63%   -0.69%     
==========================================
  Files        1721     1526     -195     
  Lines      118563   104742   -13821     
  Branches    37999    31696    -6303     
==========================================
- Hits        18170    15333    -2837     
+ Misses      79657    72495    -7162     
+ Partials    20736    16914    -3822     
Flag Coverage Δ *Carryforward flag
differential 0.00% <ø> (?)
total 14.65% <ø> (-0.68%) ⬇️ Carriedforward from 517bd91

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

@kminoda
Copy link
Contributor

kminoda commented Aug 22, 2023

@PhoebeWu21 (cc: @YamatoAndo)
Thank you for your contribution 😄 . The overall idea of this PR looks good to me 👍

However, if you don't mind, can we halt this PR and leave this package as it is for a short period of time?

The main reason is that this modification may change how parameters are reflected to gnss_poser in various project going on in Autoware Foundation. If we extract some parameters (like coordinate_system, which is reflected in sample_sensor_kit_launch) to yaml file, coordinate_system will not be reflected any longer, which would change the whole system behavior. It is possible that the similar situation may exists in many Autoware projects.

Essentially, this comes from the duplicated parameter maintenance of some parameters like coordinate_system, which ideally should be handled with map files (in map_projection_loader). In other words, current gnss_poser parameters are not optimal, but rather in a "temporary" state. That being said, I propose to first do a major refactoring on gnss_poser to remove all such parameters (coordinate_system, plane_zone, latitude, etc), and then extract the parameters as well as introducing JSON Schema as proposed in this PR. This should save us the additional cost of going through some other packages to mitigate the impact on current Autoware projects.

If you don't mind, I will handle the above major refactoring by myself, so would you do me a favor of holding this PR for a moment? It shouldn't take that much of time, but may take for several weeks if things gets complicated. But definitely doable.

@PhoebeWu21
Copy link
Member Author

@PhoebeWu21 (cc: @YamatoAndo) Thank you for your contribution 😄 . The overall idea of this PR looks good to me 👍

However, if you don't mind, can we halt this PR and leave this package as it is for a short period of time?

The main reason is that this modification may change how parameters are reflected to gnss_poser in various project going on in Autoware Foundation. If we extract some parameters (like coordinate_system, which is reflected in sample_sensor_kit_launch) to yaml file, coordinate_system will not be reflected any longer, which would change the whole system behavior. It is possible that the similar situation may exists in many Autoware projects.

Essentially, this comes from the duplicated parameter maintenance of some parameters like coordinate_system, which ideally should be handled with map files (in map_projection_loader). In other words, current gnss_poser parameters are not optimal, but rather in a "temporary" state. That being said, I propose to first do a major refactoring on gnss_poser to remove all such parameters (coordinate_system, plane_zone, latitude, etc), and then extract the parameters as well as introducing JSON Schema as proposed in this PR. This should save us the additional cost of going through some other packages to mitigate the impact on current Autoware projects.

If you don't mind, I will handle the above major refactoring by myself, so would you do me a favor of holding this PR for a moment? It shouldn't take that much of time, but may take for several weeks if things gets complicated. But definitely doable.

Got it. Okay!

@stale
Copy link

stale bot commented Oct 22, 2023

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

@stale stale bot added the status:stale Inactive or outdated issues. (auto-assigned) label Oct 22, 2023
@YamatoAndo
Copy link
Contributor

@kminoda Has your refactoring been completed? If it's done, please let us know.

@stale stale bot removed the status:stale Inactive or outdated issues. (auto-assigned) label Nov 15, 2023
@kminoda
Copy link
Contributor

kminoda commented Nov 15, 2023

@YamatoAndo Thank you for the reminder 🙏
@PhoebeWu21 It is now complete, so you may restart working on this PR, thanks

@PhoebeWu21
Copy link
Member Author

PhoebeWu21 commented Nov 15, 2023

@YamatoAndo Thank you for the reminder 🙏 @PhoebeWu21 It is now complete, so you may restart working on this PR, thanks

@kminoda Okay, thanks!

@github-actions github-actions bot added the type:documentation Creating or refining documentation. (auto-assigned) label Nov 15, 2023
@kminoda kminoda merged commit 00dedb7 into autowarefoundation:main Nov 24, 2023
26 of 29 checks passed
@PhoebeWu21 PhoebeWu21 deleted the refactor-node-config-gnss_poser branch November 24, 2023 00:45
danielsanchezaran pushed a commit to tier4/autoware.universe that referenced this pull request Dec 15, 2023
* refactor(gnss_poser): rework parameters

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

* refactor(gnss_poser): rework parameters

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

* style(pre-commit): autofix

* refactor(gnss_poser): rework parameters

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

* refactor(gnss_poser): rework parameters

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

* refactor(gnss_poser): rework parameters

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

---------

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: Yamato Ando <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:sensing Data acquisition from sensors, drivers, preprocessing. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants