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

Update lane change mode #948

Merged
merged 4 commits into from
Jun 11, 2020
Merged

Update lane change mode #948

merged 4 commits into from
Jun 11, 2020

Conversation

Yasharzf
Copy link
Collaborator

Existing lane change modes are ambiguous and misleading. For instance, "strategic" lane change mode description says "Human cars make lane changes in accordance with SUMO to provide speed boosts" however, the assigned bitset is 1621 which is SUMO's default lane change mode (execute all changes unless in conflict with TraCI). Or the bitset for "aggressive" mode is 0 which in addition to deactivating safety checks, deactivates all lane changes. I added additional lane change modes.

@Yasharzf Yasharzf changed the title Yashar lc mode Update lane change mode May 27, 2020
Comment on lines 920 to 922
* "no_lc_safe" (default): Disable all autonomous changing but still
handle safety checks (collision avoidance and safety-gap enforcement)
in the simulation. Binary is [001000000000]
Copy link
Member

Choose a reason for hiding this comment

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

We need to be careful about the word autonomous, this implies to me that the AVs cannot lane change. I think it's worth expanding the text here to be clearer about the effects.

Copy link
Member

Choose a reason for hiding this comment

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

I think if you say "disable all SUMO lane changing" that might be good

Comment on lines 927 to 928
* "sumo_default": Execute all changes unless in conflict with TraCI.
Binary is [011001010101].
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you make clear what "all" refers to?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6309

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 89.304%

Totals Coverage Status
Change from base Build 6301: 0.02%
Covered Lines: 9468
Relevant Lines: 10602

💛 - Coveralls

@AboudyKreidieh
Copy link
Member

@eugenevinitsky I've addressed your comments in this PR. everything else looks good to me, let me know if you'd like to address anything else before we merge it

@AboudyKreidieh AboudyKreidieh merged commit 67dc121 into master Jun 11, 2020
@AboudyKreidieh AboudyKreidieh deleted the yashar_lc_mode branch June 11, 2020 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants