-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add reset options to "hiway-v1". #1862
Conversation
@property | ||
def smarts(self): | ||
"""Gives access to the underlying simulator. Use this carefully. | ||
|
||
Returns: | ||
smarts.core.smarts.SMARTS: The smarts simulator instance. | ||
""" | ||
return self._smarts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering we have previously provided def scenario()
, what is the use case for additionally providing def smarts()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally just for direct access to SMARTS, I am somewhat cautious about allowing this but if we do not allow it there may be little exposure to actually using SMARTS directly.
I am partially hoping people will be tempted to use it.
observation_options="unformatted", | ||
headless=True, | ||
visdom=False, | ||
fixed_timestep_sec=0.01, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should simply make the fixed_timestep_sec=0.01
as the default value in HiWayEnvV1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 0.01
? The fixed time-step for the environment is 0.1
if not already given.
scenarios = ["scenarios/sumo/loop"] | ||
scenario: Scenario = next(Scenario.scenario_variations(scenarios, [AGENT_ID])) | ||
|
||
env.reset(options={"scenario": scenario, "start_time": 100}) | ||
assert "loop" in env.scenario.root_filepath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the env originally only had one scenario, namely scenarios/sumo/loop
, testing reset functionality by resetting to the same scenario might be an insufficient test. We could try resetting the env to a different scenario to check whether the functionality works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I was grabbing something quick, I will try "cloverleaf".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used figure_eight
.
assert not (agent_ids - set(env.agent_interfaces)) | ||
matching_items = [ | ||
env.agent_interfaces[k] == agent_interfaces[k] | ||
for k in env.agent_interfaces | ||
if k in agent_interfaces | ||
] | ||
assert all(matching_items) | ||
assert len(env.agent_interfaces) == len(agent_interfaces) | ||
assert not (agent_ids - env.agent_ids) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering whether these checks could be simplified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also note that these checks should not be in the fixture. I am moving and simplifying them.
See CHANGELOG.md