Skip to content

Add flag auto_color flag to SumoParams and disable coloring if flag is off.#849

Merged
AboudyKreidieh merged 7 commits intomasterfrom
type_vehicle_coloring
Mar 5, 2020
Merged

Add flag auto_color flag to SumoParams and disable coloring if flag is off.#849
AboudyKreidieh merged 7 commits intomasterfrom
type_vehicle_coloring

Conversation

@kanaadp
Copy link
Copy Markdown
Collaborator

@kanaadp kanaadp commented Mar 4, 2020

Pull request information

  • Status: ? Ready to Merge
  • Kind of changes: ? New Feature
  • Related PR or issue: ? N/A

Description

Added check in traci_kernel to turn off auto_coloring (for sumo). This allows us to specify custom colors for vehicle types, inflows, and routes.

i.e.
Screen Shot 2020-03-04 at 12 19 16 PM

Colors can be set via:
Screen Shot 2020-03-04 at 12 19 28 PM

or

Screen Shot 2020-03-04 at 12 19 45 PM

@coveralls
Copy link
Copy Markdown

coveralls commented Mar 4, 2020

Pull Request Test Coverage Report for Build 5265

  • 10 of 12 (83.33%) changed or added relevant lines in 2 files are covered.
  • 28 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 89.901%

Changes Missing Coverage Covered Lines Changed/Added Lines %
flow/core/params.py 2 4 50.0%
Files with Coverage Reduction New Missed Lines %
flow/controllers/velocity_controllers.py 28 69.37%
Totals Coverage Status
Change from base Build 5208: -0.2%
Covered Lines: 9000
Relevant Lines: 10011

💛 - Coveralls

@eugenevinitsky
Copy link
Copy Markdown
Member

It seems like auto_color_vehicles is unnecessary and likely to cause confusion. Why not just leave default coloring on and override it if a color is specified?

@eugenevinitsky
Copy link
Copy Markdown
Member

Otherwise, great minds think alike: #848

Comment on lines +285 to +286
if color:
type_params['color'] = color
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This works?! That's amazing!

@AboudyKreidieh
Copy link
Copy Markdown
Member

We're gonna end up in a situation here where one feature (color) will not work unless another default parameter is changed (in this case auto_color_vehicles). Should we default auto_color_vehicles to False, stick with the earlier version of this PR, or are we okay with this?

@eugenevinitsky
Copy link
Copy Markdown
Member

This is an improvement on my PR, but, I think we can just remove auto_color_vehicles completely and just override it if and only if a color is provided.

@kanaadp
Copy link
Copy Markdown
Collaborator Author

kanaadp commented Mar 5, 2020

you'd have to check if any inflow, vehicle, or route has a color defined before overriding, which is pretty cumbersome. I want to avoid setting the color every step if possible though, because it can cause flashing and that looks pretty bad

@eugenevinitsky
Copy link
Copy Markdown
Member

eugenevinitsky commented Mar 5, 2020

Or you can just have color default to None and if it's None you don't override. Or something of that sort.

@eugenevinitsky
Copy link
Copy Markdown
Member

Some mechanism is needed, expecting people to remember to set auto_color_vehicles to false when you add colors will cause a lot of confusion,.

Copy link
Copy Markdown
Member

@eugenevinitsky eugenevinitsky left a comment

Choose a reason for hiding this comment

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

LGTM!

try:
# color rl vehicles red
self.set_color(veh_id=veh_id, color=RED)
if force_update or 'color' not in self.type_parameters[self.get_type(veh_id)]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add one comment explaining this line. It's the one thing that's confusing

try:
color = CYAN if veh_id in self.get_observed_ids() else WHITE
self.set_color(veh_id=veh_id, color=color)
if force_update or 'color' not in self.type_parameters[self.get_type(veh_id)]:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto here.

@kanaadp
Copy link
Copy Markdown
Collaborator Author

kanaadp commented Mar 5, 2020

@eugenevinitsky and @AboudyKreidieh made it a lil cleaner, lmk

Copy link
Copy Markdown
Member

@AboudyKreidieh AboudyKreidieh left a comment

Choose a reason for hiding this comment

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

LGTM

@AboudyKreidieh AboudyKreidieh merged commit 7d632c7 into master Mar 5, 2020
@AboudyKreidieh AboudyKreidieh deleted the type_vehicle_coloring branch March 5, 2020 04:56
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