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.

Sign up for GitHub

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

Jump to bottom

fix(min-velocity-map-based-prediction): reduce min_velocity_for_map_based_prediction #994

Merged
merged 1 commit into from
Aug 30, 2024
Merged

fix(min-velocity-map-based-prediction): reduce min_velocity_for_map_based_prediction #994

merged 1 commit into from
Aug 30, 2024

Conversation

ahmeddesokyebrahim
Copy link
Contributor

@ahmeddesokyebrahim ahmeddesokyebrahim commented May 21, 2024 •
edited
Loading

Description

Fixes autowarefoundation/autoware.universe#7080

Related links

Parent Issue:

How was this PR tested?

Using scenario simulation

Tests performed

As it obvious in the issue that the failing iteration is when the NPC is moving with 4.5 km/h (1.25 m/s). So the following videos are testing this PR for that specific iteration

Before this PR :

2024-07-18.18-36-24.mp4

After this PR :

2024-07-18.18-40-41.mp4

For testing the PR using scenario simulation, you can use the these scenario and map files.

Notes for reviewers

Interface changes

ROS Topic Changes

N.A

ROS Parameter Changes

Parameter Name Default Value Update Description
min_velocity_for_map_based_prediction 1.0 lowering minimum velocity that allows prediction to be generated so that intersection module can handle low speed NPCs

Effects on system behavior

After this change, autoware_behavior_velocity_intersection_module should be able to check the upcoming low speed NPC and stop for it

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.

@ahmeddesokyebrahim ahmeddesokyebrahim self-assigned this May 21, 2024
@github-actions github-actions bot added the component:map Map creation, storage, and loading. (auto-assigned) label May 21, 2024
@kminoda
Copy link
Contributor

kminoda commented May 21, 2024 •
edited
Loading

Hi, thank you for the PR!
Would you clarify why you would like to change the parameter not only in your repos but for awf/autoware? Seems that the parameter is tuned for your case, but does not explain if the modified parameter is useful for the other cases in general. So, if there is any valid reason for merging this PR into awf/autoware_launch, please write down the explanation in the description 👍

Also, please consider not changing the value and just maintain your tuned parameter in your forked repository.

@ahmeddesokyebrahim ahmeddesokyebrahim force-pushed the autoware/launch/7080-fix-min-velocity-map-based-prediction branch from 01f85c8 to 8dcc04a Compare May 21, 2024 04:04
@ahmeddesokyebrahim
Copy link
Contributor Author

Hi @kminoda -san
Thanks for your comment.

Would you clarify why you would like to change the parameter not only in your repos but for awf/autoware? Seems that the parameter is tuned for your case, but does not explain if the modified parameter is useful for the other cases in general. So, if there is any valid reason for merging this PR into awf/autoware_launch, please write down the explanation in the description 👍

The description in this issue explains the problem with videos attached.
In the PR, you can find a video with impact after the change.

Also, please consider not changing the value and just maintain your tuned parameter in your forked repository.

I am doing the change in my forked repo and the purpose of this PR is to merge the branch to awf main instead of having a lot of work branches in awf repos.

Please let me know if you there is any point needs more clarification.

Thanks again :)

@ahmeddesokyebrahim ahmeddesokyebrahim mentioned this pull request May 23, 2024
Closed
5 tasks
@ahmeddesokyebrahim
Copy link
Contributor Author

Hi @kminoda -san, @YoshiRi -san, @miursh -san

I have provided for this PR a detailed context explanation in this issue. As well, you can find in this PR description video demo with the failing tests before/after this PR.

As well, we had a discussion for this change with @soblin -san and he has approved the change. You can have a look this discussion in the issue comments.

Please let me know if there is any further information needed for your review.

cc: @mitsudome-r -san

@ahmeddesokyebrahim ahmeddesokyebrahim force-pushed the autoware/launch/7080-fix-min-velocity-map-based-prediction branch from 8dcc04a to 908b023 Compare August 20, 2024 13:23
@ahmeddesokyebrahim
Copy link
Contributor Author

Hi @kminoda -san, @YoshiRi -san, @miursh -san

This is a kind reminder that this PR is waiting for your review for a very long time.

cc : @mitsudome-r -san

fix(min-velocity-map-based-prediction): reduce min_velocity_for_map_b…
44bcfb9 
…ased_prediction to let intersection module run with low speed npc

Signed-off-by: Ahmed Ebrahim <[email protected]>
@ahmeddesokyebrahim ahmeddesokyebrahim force-pushed the autoware/launch/7080-fix-min-velocity-map-based-prediction branch from 908b023 to 44bcfb9 Compare August 29, 2024 13:08
@YoshiRi
Copy link
Contributor

YoshiRi commented Aug 29, 2024

@ahmeddesokyebrahim
Sorry, I completely forgot to catch up with this issue.
@soblin
Did you checked this PR with our scenarios? If it makes no problems I will immediately approve this.

YoshiRi
YoshiRi approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@YoshiRi YoshiRi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@ahmeddesokyebrahim Thank you for your work and patience 🙇
Now we confirmed with our scenarios.
evidence: https://evaluation.tier4.jp/evaluation/reports/12892709-9980-5fd4-b1c0-4a6ffe700b1b?project_id=prd_jt

LGTM

@YoshiRi YoshiRi merged commit abb7dea into autowarefoundation:main Aug 30, 2024
11 checks passed
@tier4-autoware-public-bot tier4-autoware-public-bot bot mentioned this pull request Aug 30, 2024
Merged
zawlali pushed a commit to HelloWorldRobotics/autoware_launch that referenced this pull request Sep 25, 2024
@ahmeddesokyebrahim @zawlali
fix(min-velocity-map-based-prediction): reduce min_velocity_for_map_b…
1c166a3 
…ased_prediction (autowarefoundation#994)

fix(min-velocity-map-based-prediction): reduce min_velocity_for_map_based_prediction to let intersection module run with low speed npc

Signed-off-by: Ahmed Ebrahim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YoshiRi YoshiRi YoshiRi approved these changes

@soblin soblin soblin approved these changes

@miursh miursh Awaiting requested review from miursh miursh is a code owner

@kminoda kminoda Awaiting requested review from kminoda kminoda is a code owner

Assignees

@ahmeddesokyebrahim ahmeddesokyebrahim

Labels
component:map Map creation, storage, and loading. (auto-assigned) tag:run-build-and-test-differential
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Dense-Urban-ODD] Intersection module is not triggered for low speed NPC
4 participants
@ahmeddesokyebrahim @kminoda @YoshiRi @soblin