-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: refactor best/worst record api in response time #323
Conversation
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
@rokamu623 I see you have some commit in the fork repository, do you need any additional changes on? |
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[email protected]>
{'start': 4, 'response_time': 6}, | ||
{'start': 6, 'response_time': 0} | ||
] | ||
assert to_dict(response_time) == expect |
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.
@rokamu623
I think need to add test for the following cases:
End Time Earlier Than Start Time:
records_raw = [
{'start': 3, 'end': 2},
{'start': 5, 'end': 4},
]
Only End Time, No Start Time:
records_raw = [
{'end': 3},
{'end': 5},
]
Signed-off-by: rokamu623 <[email protected]>
Signed-off-by: rokamu623 <[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.
I used "autoware_launch_trace_20221227-150253_universe_rosbag" to get the timeseries graph of ResponseTime before and after applying this PR. Before applying this PR, the version was bokeh3.3.0.
Visual inspection showed that there were differences in "best" and "worst-with-external-latency" cases. Please check the difference.
@xygyo77 Regarding this, In fact, except in the Therefore, after refactoring, the response time for the 1st path is recorded more than before refactoring. |
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
Note: With https://tier4.atlassian.net/browse/RT2-888, the "worst-with-external-latency" case has been replaced by the old "worst" case.
)" This reverts commit 6482159.
Description
The method of calculating ResponseTime for the
best
andworst-with-external-latency
cases has been changed to be the same as forall
andworst
case.Related links
Jira: https://tier4.atlassian.net/browse/RT2-889
Notes for reviewers
Pre-review checklist for the PR author
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.