Skip to content

I210 highway updated #934

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
Merged
merged 17 commits into from
Jun 15, 2020
Merged

I210 highway updated #934

merged 17 commits into from
Jun 15, 2020

Conversation

AboudyKreidieh
Copy link
Member

Pull request information

  • Status: ? (ready to merge / in development)
  • Kind of changes: ? (bug fix / new feature / documentation...)
  • Related PR or issue: ? (optional)

Description

? (general description)

@coveralls
Copy link

coveralls commented Jun 10, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 6426

  • 70 of 75 (93.33%) changed or added relevant lines in 7 files are covered.
  • 51 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.2%) to 88.972%
Changes Missing Coverage Covered Lines Changed/Added Lines %
flow/controllers/routing_controllers.py 7 8 87.5%
examples/exp_configs/non_rl/i210_subnetwork.py 14 18 77.78%
Files with Coverage Reduction New Missed Lines %
flow/controllers/car_following_models.py 1 95.15%
flow/visualize/time_space_diagram.py 24 72.28%
flow/core/params.py 26 89.47%
Totals Coverage Status
Change from base Build 6322: -0.2%
Covered Lines: 9423
Relevant Lines: 10591
💛 - Coveralls

@AboudyKreidieh
Copy link
Member Author

@eugenevinitsky @kjang96 this PR is ready to be reviewed. Can one/both of you review this?

This PR just covers updating the highway network and getting the features for the I-210 subnetwork in here; the actual working version of the I-210, along with the time-space diagram that generate proper plots for it, will be part of a separate PR.

kjang96
kjang96 requested changes Jun 11, 2020
View reviewed changes
Copy link
Member

@kjang96 kjang96 left a comment

Choose a reason for hiding this comment

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

Mostly nitpicky stuff, the tests look really good, and cover the things I would have manually run

examples/exp_configs/rl/multiagent/multiagent_i210.py
# percentage of autonomous vehicles compared to human vehicles on highway
PENETRATION_RATE = 10

# TODO: temporary fix
Copy link
Member

Choose a reason for hiding this comment

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

Still a TODO?

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, this will be part of a future PR probably

examples/exp_configs/rl/multiagent/multiagent_i210.py
net=NetParams(
inflows=inflow,
template=NET_TEMPLATE
template=NET_TEMPLATE,
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, can you go through and change instances of NET_TEMPLATE and net_template such that they're either all in upper or lower case? I noticed in non_rl/i210_subnetwork.py and non_rl/highway_single.py they're lower

Copy link
Member Author

Choose a reason for hiding this comment

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

done, they're all upper case now

flow/networks/highway.py
# speed limit for the ghost edge
"ghost_speed_limit": 25,
# length of the cell imposing a boundary
"boundary_cell_length": 500
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 this needs a more descriptive description

Copy link
Member Author

Choose a reason for hiding this comment

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

done, linked it to the ghost cell description

flow/networks/highway.py Outdated
* **use_ghost_edge** : whether to include a ghost edge. This edge is
provided a different speed limit.
* **ghost_speed_limit** : speed limit for the ghost edge
* **boundary_cell_length** : length of the cell imposing a boundary
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@AboudyKreidieh AboudyKreidieh requested a review from kjang96 June 15, 2020 18:19
@AboudyKreidieh
Copy link
Member Author

thanks for the review @kjang96! I've addressed all your comments, and going to merge it now.

@AboudyKreidieh AboudyKreidieh merged commit 86c2e4c into master Jun 15, 2020
@AboudyKreidieh AboudyKreidieh deleted the i210-highway-updated branch June 15, 2020 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cathywu cathywu Awaiting requested review from cathywu

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

@kanaadp kanaadp Awaiting requested review from kanaadp

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AboudyKreidieh @coveralls @kjang96