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(localization_error_monitor): refactor diag #4964

Conversation

YamatoAndo
Copy link
Contributor

@YamatoAndo YamatoAndo commented Sep 12, 2023

Description

I refactored localization_error_monitor's diagnostics.

  • refactor localization_error_monitor's diagnostics code
  • add test code for diagnostics code
  • add the diagnostics image to the readme
  • rename diagnostic_aggregator's param

And I have consolidated what was originally output separately into one.

before

Screenshot from 2023-09-12 18-46-47

Screenshot from 2023-09-12 18-46-56


after
diagnostics

Related links

N/A

Tests performed

  • LSim works well.
  • check localization_error_monitor's diagnostics by using ros2 run rqt_runtime_monitor rqt_runtime_monitor

Notes for reviewers

N/A

Interface changes

N/A

Effects on system behavior

N/A

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.

yamato-ando added 10 commits September 8, 2023 19:28
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:localization Vehicle's position determination in its environment. (auto-assigned) labels Sep 12, 2023
@YamatoAndo YamatoAndo added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Sep 12, 2023
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

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

Comparison is base (c552c3d) 14.85% compared to head (56d5faa) 15.78%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4964      +/-   ##
==========================================
+ Coverage   14.85%   15.78%   +0.92%     
==========================================
  Files        1627     1579      -48     
  Lines      112656   108672    -3984     
  Branches    34806    33576    -1230     
==========================================
+ Hits        16739    17150     +411     
+ Misses      77156    72916    -4240     
+ Partials    18761    18606     -155     
Flag Coverage Δ *Carryforward flag
differential 7.77% <31.29%> (?)
total 15.77% <ø> (+0.92%) ⬆️ Carriedforward from d2a263a

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
...onitor/include/localization_error_monitor/node.hpp 0.00% <ø> (ø)
...calization/localization_error_monitor/src/node.cpp 0.00% <0.00%> (ø)
...ion/localization_error_monitor/src/diagnostics.cpp 60.97% <60.97%> (ø)
...calization_error_monitor/test/test_diagnostics.cpp 20.25% <20.25%> (ø)

... and 352 files with indirect coverage changes

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

Copy link
Contributor

@KYabuuchi KYabuuchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactoring is basically good. But some parts are difficult to read.
Please check my following comments.

YamatoAndo and others added 2 commits September 13, 2023 14:00
Signed-off-by: yamato-ando <Yamato ANDO>
@KYabuuchi
Copy link
Contributor

We need to modify system/system_error_monitor/config/diagnostic_aggregator/localization.param.yaml too.

The condition needs to be changed from contains: [": localization_accuracy"] to contains: ["localization_accuracy"] in order to aggregate the concatenated diagnostics.

yamato-ando added 2 commits September 13, 2023 15:31
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
@YamatoAndo YamatoAndo requested a review from ito-san as a code owner September 13, 2023 06:34
@github-actions github-actions bot added the component:system System design and integration. (auto-assigned) label Sep 13, 2023
@YamatoAndo
Copy link
Contributor Author

@ito-san
I changed the component name from localization_error_monitor: localization_accuracy to localization: localization_error_monitor, so I updated the diagnostic_aggregator's param.
Please review this PR.

yamato-ando added 3 commits September 13, 2023 15:57
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
Signed-off-by: yamato-ando <Yamato ANDO>
@YamatoAndo
Copy link
Contributor Author

@KYabuuchi I have made a change so that when the diagnostic status is "OK," the message "OK" will be displayed.

Copy link
Contributor

@SakodaShintaro SakodaShintaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good To Me

Copy link
Contributor

@KYabuuchi KYabuuchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@KYabuuchi
Copy link
Contributor

RELATED LINK: autowarefoundation/autoware_launch#565
(It update default parameter of localization_error_monitor)

@ito-san ito-san added the type:new-feature New functionalities or additions, feature requests. label Sep 14, 2023
Copy link
Contributor

@ito-san ito-san left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@YamatoAndo YamatoAndo enabled auto-merge (squash) September 14, 2023 07:39
@YamatoAndo
Copy link
Contributor Author

@Motsu-san Could you please approve this pr?

Copy link
Contributor

@Motsu-san Motsu-san left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to understand why this change is needed and how influences on localization_error_monitor.

@Motsu-san
Copy link
Contributor

@YamatoAndo After you resolve my question and conflicts, I will approve.

@YamatoAndo YamatoAndo requested a review from kminoda as a code owner September 28, 2023 02:29
@YamatoAndo YamatoAndo disabled auto-merge September 28, 2023 02:32
@YamatoAndo YamatoAndo removed the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Sep 28, 2023
Signed-off-by: yamato-ando <Yamato ANDO>
@YamatoAndo YamatoAndo added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Sep 28, 2023
Copy link
Contributor

@Motsu-san Motsu-san left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMeow

@YamatoAndo YamatoAndo merged commit fe9ad68 into autowarefoundation:main Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:localization Vehicle's position determination in its environment. (auto-assigned) component:system System design and integration. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) type:documentation Creating or refining documentation. (auto-assigned) type:new-feature New functionalities or additions, feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants