feat(ndt_scan_matcher): adding exe time to diagnostics for checking runtime_monitor #4916

yvzksgl · 2023-09-07T10:03:39Z

Description

This PR offersto add exe_time to diagnostics.

Exe time is useful for observe does NDT have problems while matching scans. I can say with my observations, if NDT exe_time is exceeded 24ms, it will be most likely started to cannot match scans properly. So while I'm checking runtime_monitor i would like to see NDT exe_time. Formerly this PR is a part of #4854.

Related: autowarefoundation/autoware_launch#559

Tests performed

All added options runned through logging simulator and test vehicle.

Effects on system behavior

It gives a facility for observing changes in nearest_voxel_transformation_likelihood and exe_time.

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 Reviewers must check the checkboxes below before approval.

The PR follows the pull request guidelines.

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.

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

kminoda

Thank you for the PR!

localization/ndt_scan_matcher/config/ndt_scan_matcher.param.yaml

kminoda

LGTM except the following part, thanks 🙏

localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp

Motsu-san · 2023-09-08T03:10:22Z

@yvzksgl
Thank you for your contribution.
I would like to respond here about this

Your suggestion of adding a condition to generate warnings is very good in itself.
However, I'm aware that the execution time of NDT varies depending on the environment.
It is difficult to approve a warning at 24 ms because there is no evidence or design for Positive/False detecting rate.
For example, there can be an emergency stop system that operates by seeing this warning, and the addition of this condition will simply have the effect of increasing the frequency of activation.
So, how about setting a default value, such as 100 ms, that can be reliably determined this time that the default update cycle cannot be maintained, so that warnings can be issued with matching threshold values in individual environments?
Or has this threshold been discussed and concluded somewhere? (pull request guidelines)

Motsu-san

I would like to avoid settings that affect the applications this warning is followed by.

localization/ndt_scan_matcher/config/ndt_scan_matcher.param.yaml

localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp

yvzksgl · 2023-09-11T08:14:15Z

I can create a PR for autoware_launch. Also if you want to discuss about exe_time warning value I can create discussion about it. However, I am okay with 100 and it makes sense to me as well.

localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp

kminoda · 2023-09-11T10:59:38Z

Please add the autoware_launch PR link in the description, and make sure that both are merged at the same time.

kminoda · 2023-09-11T11:28:43Z

Please fix the following

Conflict
semantic pull request (PR titles should not include capital letters)
pre-commit (fix all the warnings listed)
DCO (I will make it pass as a maintainer after LGTM, so no need to deal with it)

Motsu-san

Thank you for your great work!

yvzksgl · 2023-09-12T10:43:17Z

Please fix the following

* Conflict

* semantic pull request (PR titles should not include capital letters)

* pre-commit (fix all the warnings listed)

* DCO (I will make it pass as a maintainer after LGTM, so no need to deal with it)

I fixed them but github still says there are conflicts. A bug should be occured on github or something, i guess. Do you mind if I ask you to take a look? Thank you.

kminoda · 2023-09-12T23:42:37Z

@yvzksgl Sure, but it may take a while since I have other things to do unfortunately.
Those CIs are just general issues and googling or chatgpt-ing may solve the issue.

Motsu-san · 2023-09-25T22:57:47Z

@yvzksgl Please resolve conflicts.

Co-authored-by: kminoda <[email protected]>

Co-authored-by: Motz <[email protected]>

codecov · 2023-09-26T17:18:07Z

Codecov Report

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

Comparison is base (19ab508) 14.89% compared to head (9163f43) 14.89%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4916      +/-   ##
==========================================
- Coverage   14.89%   14.89%   -0.01%     
==========================================
  Files        1625     1625              
  Lines      112449   112460      +11     
  Branches    34712    34712              
==========================================
  Hits        16752    16752              
- Misses      76959    76970      +11     
  Partials    18738    18738

Flag	Coverage Δ		*Carryforward flag
differential	`0.00% <0.00%> (?)`
total	`14.89% <ø> (+<0.01%)`	⬆️	Carriedforward from 19ab508

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

Files	Coverage Δ
...ion/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp	`0.00% <0.00%> (ø)`

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

…untime_monitor (autowarefoundation#4916) * adding exe time to diagnostics * Update localization/ndt_scan_matcher/config/ndt_scan_matcher.param.yaml Co-authored-by: kminoda <[email protected]> * order of the parameters in constructor corrected * spell check * Update localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp Co-authored-by: kminoda <[email protected]> * Update localization/ndt_scan_matcher/config/ndt_scan_matcher.param.yaml Co-authored-by: Motz <[email protected]> * Update localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp Co-authored-by: Motz <[email protected]> * rebase branch to main * the actual execution time added to diagnostics message * execution_time parameter corrected * integration with header file * style(pre-commit): autofix * parameter naming --------- Co-authored-by: kminoda <[email protected]> Co-authored-by: Motz <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

yvzksgl requested review from KYabuuchi, kminoda, Motsu-san and YamatoAndo as code owners September 7, 2023 10:03

github-actions bot added the component:localization Vehicle's position determination in its environment. (auto-assigned) label Sep 7, 2023

kminoda reviewed Sep 7, 2023

View reviewed changes

localization/ndt_scan_matcher/config/ndt_scan_matcher.param.yaml Outdated Show resolved Hide resolved

yvzksgl marked this pull request as draft September 7, 2023 10:14

yvzksgl marked this pull request as ready for review September 7, 2023 10:26

kminoda reviewed Sep 8, 2023

View reviewed changes

localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp Outdated Show resolved Hide resolved

localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp Outdated Show resolved Hide resolved

Motsu-san requested changes Sep 11, 2023

View reviewed changes

localization/ndt_scan_matcher/config/ndt_scan_matcher.param.yaml Outdated Show resolved Hide resolved

localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp Outdated Show resolved Hide resolved

kminoda reviewed Sep 11, 2023

View reviewed changes

localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp Outdated Show resolved Hide resolved

yvzksgl changed the title ~~feat(ndt_scan_matcher): Adding exe time to diagnostics: For checking runtime_monitor~~ feat(ndt_scan_matcher): Adding exe time to diagnostics for checking runtime_monitor Sep 11, 2023

yvzksgl mentioned this pull request Sep 11, 2023

feat(ndt_scan_matcher): adding exe time parameter autowarefoundation/autoware_launch#559

Merged

4 tasks

yvzksgl changed the title ~~feat(ndt_scan_matcher): Adding exe time to diagnostics for checking runtime_monitor~~ feat(ndt_scan_matcher): adding exe time to diagnostics for checking runtime_monitor Sep 11, 2023

Motsu-san approved these changes Sep 11, 2023

View reviewed changes

YamatoAndo mentioned this pull request Sep 22, 2023

feat(ndt_scan_matcher): remake diag #5076

Merged

10 tasks

yvzksgl and others added 7 commits September 26, 2023 11:19

adding exe time to diagnostics

0a053e2

Update localization/ndt_scan_matcher/config/ndt_scan_matcher.param.yaml

d5df874

Co-authored-by: kminoda <[email protected]>

order of the parameters in constructor corrected

0d94885

spell check

ff3830d

Update localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp

4048afd

Co-authored-by: kminoda <[email protected]>

Update localization/ndt_scan_matcher/config/ndt_scan_matcher.param.yaml

0a8935a

Co-authored-by: Motz <[email protected]>

Update localization/ndt_scan_matcher/src/ndt_scan_matcher_core.cpp

e0dab9d

Co-authored-by: Motz <[email protected]>

yvzksgl added 4 commits September 26, 2023 11:21

rebase branch to main

8d1d1bf

the actual execution time added to diagnostics message

122ac3d

execution_time parameter corrected

8642592

integration with header file

cd5a8c8

yvzksgl force-pushed the feat/ndt-diagnotics-exe-time branch from eeef736 to cd5a8c8 Compare September 26, 2023 08:38

pre-commit-ci bot and others added 2 commits September 26, 2023 08:41

style(pre-commit): autofix

216f1ef

parameter naming

7351359

yvzksgl enabled auto-merge (squash) September 26, 2023 09:08

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

Merge branch 'main' into feat/ndt-diagnotics-exe-time

9163f43

yvzksgl merged commit 3731fd0 into main Sep 27, 2023
21 of 25 checks passed

yvzksgl deleted the feat/ndt-diagnotics-exe-time branch September 27, 2023 04:16

SakodaShintaro mentioned this pull request Sep 27, 2023

fix(ndt_scan_matcher): removed duplicate parameter declarations "initial_pose_timeout_sec_" #5153

Merged

4 tasks

This was referenced Oct 11, 2023

chore: sync upstream tier4/autoware.universe#927

Merged

chore: sync upstream tier4/autoware.universe#928

Merged

shmpwk mentioned this pull request Nov 16, 2023

fix(ndt_scan_matcher): delete diagnostic thread and its dependencies tier4/autoware.universe#1021

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ndt_scan_matcher): adding exe time to diagnostics for checking runtime_monitor #4916

feat(ndt_scan_matcher): adding exe time to diagnostics for checking runtime_monitor #4916

yvzksgl commented Sep 7, 2023 •

edited

Loading

kminoda left a comment

kminoda left a comment

Motsu-san commented Sep 8, 2023 •

edited

Loading

Motsu-san left a comment

yvzksgl commented Sep 11, 2023

kminoda commented Sep 11, 2023

kminoda commented Sep 11, 2023

Motsu-san left a comment

yvzksgl commented Sep 12, 2023 •

edited

Loading

kminoda commented Sep 12, 2023

Motsu-san commented Sep 25, 2023

codecov bot commented Sep 26, 2023 •

edited

Loading

feat(ndt_scan_matcher): adding exe time to diagnostics for checking runtime_monitor #4916

feat(ndt_scan_matcher): adding exe time to diagnostics for checking runtime_monitor #4916

Conversation

yvzksgl 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

kminoda left a comment

Choose a reason for hiding this comment

kminoda left a comment

Choose a reason for hiding this comment

Motsu-san commented Sep 8, 2023 • edited Loading

Motsu-san left a comment

Choose a reason for hiding this comment

yvzksgl commented Sep 11, 2023

kminoda commented Sep 11, 2023

kminoda commented Sep 11, 2023

Motsu-san left a comment

Choose a reason for hiding this comment

yvzksgl commented Sep 12, 2023 • edited Loading

kminoda commented Sep 12, 2023

Motsu-san commented Sep 25, 2023

codecov bot commented Sep 26, 2023 • edited Loading

Codecov Report

yvzksgl commented Sep 7, 2023 •

edited

Loading

Motsu-san commented Sep 8, 2023 •

edited

Loading

yvzksgl commented Sep 12, 2023 •

edited

Loading

codecov bot commented Sep 26, 2023 •

edited

Loading