-
Notifications
You must be signed in to change notification settings - Fork 193
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
traffic history improvements for imitation learning #741
Conversation
- better smoothing of heading for very low vehicle speeds to reduce "wiggle" - added ability to set agent speed to imitation learning replacement example - added option to convert old JSON traffic histories to new sqlite .shf files - quiet netconvert's "Success" output when shifting maps - minor refactor of genhistories database creation code
smarts/core/renderer.py
Outdated
@@ -58,6 +58,7 @@ def __new__(cls): | |||
# disable vsync otherwise we are limited to refresh-rate of screen | |||
loadPrcFileData("", "sync-video false") | |||
loadPrcFileData("", "model-path %s" % os.getcwd()) | |||
loadPrcFileData("", "model-cache-dir %s/.panda3d_cache" % os.getcwd()) |
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.
is this related to replaying traffic data?
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.
Oops. No, it's not. I accidentally left that in after an unrelated test. I'll take it back out. Thanks for catching that!
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.
(It did speed up rendering though, so we might consider adding it at some point.)
smarts/core/sumo_road_network.py
Outdated
self._log = logging.getLogger(self.__class__.__name__) | ||
self._graph = graph | ||
self._net_file = net_file | ||
self._default_lane_width = ( | ||
default_lane_width if default_lane_width is not None else 3.2 |
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.
should we add some explanation for 3.2 here?
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.
Yes, I will add a comment.
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.
This is fine too but I might have just preferred a descriptive constant.
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.
ok, done.
genhistories_py = os.path.join( | ||
os.path.dirname(os.path.realpath(__file__)), "genhistories.py" | ||
) | ||
for hdsr in histories_datasets: |
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 am wondering what does hdsr
and hds
stand for?...
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.
hds -> "history data set" and hdsr -> "history data set ref" (or something like that!) :)
@@ -146,6 +146,7 @@ def _clean(scenario): | |||
"social_agents/*", | |||
"traffic/*", | |||
"history_mission.pkl", |
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.
Since we are using .shf
files now we can probably remove "history_mission.pkl"
here
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.
Yeah, definitely we need to eventually, but I wanted to leave it there a little bit to get rid of any existing files on the next clean.
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.
Cool! One other thing we can remove is the ijson
dependency. line 64 and 65 in setup.py and line 48 in requirements.txt. ijson was only used for imitation learning
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.
funny, I did, but I had to add it back. It's still being used in genhistories.py
to convert old json files.
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.
Nice change!
where could I take a look at how NGSIM and Interaction dataset scenario folder looks now? |
I have them locally if you want me to zip them up and send them. (I don't think we should push them here though.) |
Thanks. Your NGSIM jupyter notebook gave me a huge head start on |
observations = smarts.reset(scenario) | ||
|
||
dones = {agent_id: False} | ||
while not dones[agent_id]: | ||
while not dones.get(agent_id, False): |
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 think this is better practice but is there any case where the default should be necessary?
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.
Yeah, I hit that in my testing. It happened early on and I can't clearly remember the circumstances, but I suspect it was related to my having the mission.start_time
wrong initially (leading to agent_id
not being started yet). So I guess, when things are correct, it's not necessary, but I added it so I wouldn't crash immediately with a key error and could debug other things first.
# TODO: the following speeds up rendering a bit... might consider it. | ||
# loadPrcFileData("", "model-cache-dir %s/.panda3d_cache" % os.getcwd()) |
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.
Does this speed up rendering or model loading?
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.
oh, right, model loading. (However, as vehicle models can be loaded during the simulation, it can still affect the step time.)
smarts/core/sumo_road_network.py
Outdated
self._log = logging.getLogger(self.__class__.__name__) | ||
self._graph = graph | ||
self._net_file = net_file | ||
self._default_lane_width = ( | ||
default_lane_width if default_lane_width is not None else 3.2 |
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.
This is fine too but I might have just preferred a descriptive constant.
smarts/core/smarts.py
Outdated
@@ -224,7 +226,12 @@ def _step(self, agent_actions): | |||
extras = dict(scores=scores) | |||
|
|||
# 8. Advance the simulation clock. | |||
self._elapsed_sim_time += dt | |||
self._elapsed_sim_time = round(self._elapsed_sim_time + dt, 3) |
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 would say rounding here is wrong since it means that people will unable to run the simulation at any step less than 5e-4. While such a small step would not normally be done, I do not think it is on us to try limit a user from attempting a micro-scale time-step.
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.
Good point. (I added it because the addition of dt
sometimes caused floating point precision issues, like an _elapsed_sim_time
equal to, say, 1.9999999
instead of 2.0
.) I just changed it to never "round too much".
smarts/core/smarts.py
Outdated
self._use_realtime_clock = scenario.traffic_history and ( | ||
not self._traffic_sim.headless or not self._envision.headless | ||
) |
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 see why the real-time option might be useful for imitation learning but I have a few concerns. My first inclination here is that it should not be attached to the traffic history because there are other uses for a realtime clock option and this binds the option to the traffic history.
Secondly, Envision is already supposed to play back at real-time and I believe this complicates fixing Envision so I do not think Envision should be included with this option.
Otherwise this is useful to sumo-gui
but there is another alternative. It is possible to send the --gui-settings-file
option to sumo-gui
to add step delay and breakpoints via a configuration file: https://sumo.dlr.de/docs/sumo-gui.html#configuration_files.
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.
Re the traffic-history limitation, I agree with you. I just was hesitant to auto-set it and change the behavior for other things that might have inadvertently set headless
to False but where no one watches the gui/envision.
Re Envision, there is still a bug there, but you're right that this "masks" the problem.
I will look into the --gui-settings-file
option...
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.
Using step delay via a --gui-settings-file
could work assuming we know (approximately) how long our steps really take. But if these will be variable, or if we'll be improving our step time, then we may have to tweak this occasionally.
I'm willing to do this, but let's chat about it on Monday first. I do see other advantages to having a near-real-time mode (in particular, asyncrhonously running alongside ROS nodes), but there may be better ways to achieve that, so I can be persuaded to leave this out for now (and/or use the sumo-gui delay setting).
self.start_time_offset = 0 | ||
self._replaced_vehicle_ids = set() | ||
self._start_time_offset = 0 | ||
self._histories_db = None |
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.
Looks like you ordered the instance variables but then added one to put it back out of order. 🤣
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.
hehe, yep! :)
# Options from NGSIM and INTERACTION currently include: | ||
# 1=motorcycle, 2=auto, 3=truck, 4=pedestrian/bicycle | ||
# But we don't yet have glb models for 1 and 4. |
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 will add this as an issue.
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.
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.
Thanks.
# But we don't yet have glb models for 1 and 4. | ||
if vehicle_type == 3: | ||
return "truck" | ||
return "passenger" |
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 see this as a potential issue that we would default to a passenger vehicle when provided a pedestrian since we do not support non-road actors yet nor do we yet want to evaluate pedestrians.
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.
Yeah, I agree. I'm adding a change to skip pedestrians for now.
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.
... although on second thought, I think we should probably just add a pedestrian model fairly soon because, in an imitation learning scenario, it could cause problems to train from a vehicle that swerves or stops suddenly for seeming no reason (if a pedestrian that triggered this behavior is not part of the simulation).
So I guess I'm not going to skip them after all. We should just do Issue #756 relatively soon instead.
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.
Yes, more-so I was worried about imitation learning attempting to take over a pedestrian because it thinks it is a passenger vehicle.
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.
Ah oh. I just added something to prevent that: only passenger cars will now be included in the agent missions used by history_vehicle_replacement_for_imitation_learning.py
.
float(self.column_val_in_row(row, "speed")) * self.scale, | ||
self.column_val_in_row(row, "lane_id"), | ||
) | ||
if not any(a is not None and np.isnan(a) for a in traj_args): |
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 think the presence of isnan
would be a value useful to warn about rather than silently skipping.
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.
yeah, in general I agree. Unfortunately, the rolling window method used to calculate the moving averages for position_x
and position_y
leaves 1-window-width of NaNs at the end. This was the easiest way to ignore those. I'll add a comment to explain that.
smarts/sstudio/genhistories.py
Outdated
# Try to match the NGSIM types... | ||
if agent_type == "car": | ||
return 2 | ||
elif agent_type == "truck": | ||
return 3 | ||
elif agent_type == "pedestrian/bicycle": | ||
return 4 |
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.
Is motorcycle also supposed to be here?
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.
oops, yes! good catch!
Changes traffic history file format from JSON to an opaque
.shf
format (currently aSQLite
file database).This reduces the size of history files by over 60%, speeds up stepping from the
TrafficHistoryProvider
to under 2ms, and fixes bugs related to issue #407 (missed and skipped samples).This obsoletes the
Traffic_history_service
(and associated unit tests of it).Traffic history database files are automatically created when a
Scenario
specifies them, as before, like:However we now support a
yaml
"dataset spec" file as input that contains a pointer to the original dataset to be converted as well as parameters related to the conversion (examples have be added to Issue #732). This converts and imports the data into the sqlite database when scenarios are first built and obsoletes the previous conversion scripts (tools/interaction_dataset_converter.py
andngsim_dataset_converter.py
).Along the way:
Closes #732, #407