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

I210 merge #839

Merged
merged 76 commits into from Mar 13, 2020
Merged

I210 merge #839

merged 76 commits into from Mar 13, 2020

Conversation

eugenevinitsky
Copy link
Member

Merge the I210 network into master so everyone has access to it.

Yasharzf and others added 30 commits February 10, 2020 14:18
Copy link
Collaborator

@kanaadp kanaadp left a comment

Choose a reason for hiding this comment

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

LGTM, some minor comments

@@ -147,7 +147,6 @@ def setup_exps_rllib(flow_params,
config["lambda"] = 0.97
config["kl_target"] = 0.02
config["num_sgd_iter"] = 10
config['clip_actions'] = False # FIXME(ev) temporary ray bug
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this fixed when we upgrade to 8.0.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. 0.7.3 fixes it too

"Currently, multiagent experiments are only supported through "\
"RLlib. Try running this experiment using RLlib: 'python train.py EXP_CONFIG'"
else:
assert False, "Unable to find experiment config!"
if flags.rl_trainer == "RLlib":
if flags.rl_trainer.lower() == "rllib":
Copy link
Collaborator

Choose a reason for hiding this comment

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

why rename it? just for aesthetics?

Copy link
Member Author

Choose a reason for hiding this comment

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

They can just type whatever they want and capitalization errors wont throw an error

Comment on lines 2 to 9

import logging
from flow.core.util import emission_to_csv
from flow.utils.registry import make_create_env
import datetime
import numpy as np
import logging
import time
import os

from flow.core.util import emission_to_csv
from flow.utils.registry import make_create_env
import matplotlib.pyplot as plt
import numpy as np
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these ordered correctly?

custom_callables : [str, lambda]
List of strings and lambda functions corresponding to some information we want
to extract from the environment. The lambda will be called at each step to extract
information from the env and it will be stored in a dict keyed by the str.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe add something that they should return a numerical quantity. i think this script breaks if the callable returns None

STEPS = 10
rdelta = 255 / STEPS
# smoothly go from red to green as the speed increases
color_bins = [[int(255 - rdelta * i), int(rdelta * i), 0] for i in range(STEPS + 1)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is cool, but SUMO does this by default btw :^)

Copy link
Member Author

Choose a reason for hiding this comment

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

How do I turn this on in SUMO?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you replace this with the correct command?

Copy link
Collaborator

Choose a reason for hiding this comment

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

non-trivial to implement, have to add a gui-settings file and manually set that as the default setting for vehicle coloring

i.e. https://sumo.dlr.de/docs/SUMO-GUI.html#configuration_files

@@ -247,6 +253,17 @@ def test_multi_highway(self):
}
self.run_exp(multiagent_highway, **kwargs)

# TODO(@evinitsky) enable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't leave this in master if it doesn't work

Copy link
Member Author

Choose a reason for hiding this comment

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

Think it's alright, we have other tests like this that we need to turn on at some point. Not perfect code but this isn't a stable project till a version release anyways

@AboudyKreidieh
Copy link
Member

still need to write tests but then we can probably merge

@eugenevinitsky
Copy link
Member Author

What do you want to test before we merge this?

@eugenevinitsky eugenevinitsky force-pushed the i210_merge branch 2 times, most recently from 54cd1b4 to e6653e1 Compare March 5, 2020 18:55
@eugenevinitsky
Copy link
Member Author

eugenevinitsky commented Mar 10, 2020

@AboudyKreidieh I have tests for everything but param_sweep.py. Are you down to merge?

@eugenevinitsky eugenevinitsky force-pushed the i210_merge branch 2 times, most recently from ec64c8c to 72ba4f4 Compare March 10, 2020 01:53
@AboudyKreidieh AboudyKreidieh self-requested a review March 12, 2020 17:28
@eugenevinitsky eugenevinitsky merged commit 80f3c47 into master Mar 13, 2020
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.

None yet

5 participants