ricky/merge conflicts 3.0 beta and 3.0#325
Conversation
riccardosavorgnan
requested review from
Aditya-Gupta26,
daphne-cornelisse and
mpragnay
Greptile SummaryThis PR merges a large batch of improvements from Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant PL as pufferl.py (eval)
participant E as WOSACEvaluator
participant VE as VecEnv (Drive)
participant C as C Simulator
PL->>E: evaluate(args, vecenv, policy)
loop Until target scenarios covered OR max_batches
E->>VE: resample_maps() [batch > 0]
VE->>C: vec_close / binding.shared / env_init / vec_reset
E->>VE: collect_ground_truth_trajectories()
VE->>C: vec_get_global_ground_truth_trajectories()<br/>(x, y, z, heading, valid, id, is_vehicle, is_track_to_predict, scenario_id)
C-->>VE: GT trajectory arrays
alt eval_mode == "policy"
E->>VE: collect_simulated_trajectories(policy)
VE->>C: vec_step (num_rollouts × sim_steps)
C-->>VE: simulated x/y/heading
else eval_mode == "ground_truth"
E->>E: repeat GT trajectories as simulated
end
E->>E: compute_metrics(gt, sim, agent_state, road_edges)
Note over E: avg log-likelihoods per scenario → exp → realism_meta_score
E->>E: track unique scenario_ids
end
E-->>PL: df_scene_level (per-scenario scores)
PL->>PL: df.mean() → aggregate metrics dict
PL->>PL: log to WandB
Last reviewed commit: b5adcd6 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
pufferlib/ocean/drive/drive.c
Outdated
| #include "../env_config.h" | ||
| #include <string.h> | ||
| #include "../env_config.h" |
There was a problem hiding this comment.
Duplicate #include "../env_config.h"
env_config.h is included twice — once on line 4 and again on line 6. Although the header's #ifndef ENV_CONFIG_H guard prevents a double-definition compilation error, the second include is clearly an artifact from the merge and should be removed.
| #include "../env_config.h" | |
| #include <string.h> | |
| #include "../env_config.h" | |
| #include "drivenet.h" | |
| #include "error.h" | |
| #include "libgen.h" | |
| #include "../env_config.h" | |
| #include <string.h> |
| if not model_files: | ||
| print("No model files found for WOSAC evaluation. Running WOSAC with random policy.") | ||
| elif len(model_files) > 0: | ||
| latest_cpt = max(model_files, key=os.path.getctime) | ||
| cmd.extend(["--load-model-path", latest_cpt]) |
There was a problem hiding this comment.
Redundant elif len(model_files) > 0 condition
After the if not model_files: branch, model_files is guaranteed to be non-empty, so elif len(model_files) > 0 is always True at that point — it's dead conditional logic. Replacing it with a plain else: makes the intent clearer and avoids the misleading branch.
Additional Comments (1)
As a result, Fix by using kinematic_cols = [
"likelihood_linear_speed",
"likelihood_linear_acceleration",
"likelihood_angular_speed",
"likelihood_angular_acceleration",
]
interactive_cols = [
"likelihood_collision_indication",
"likelihood_distance_to_nearest_object",
"likelihood_time_to_collision",
]
map_cols = [
"likelihood_distance_to_road_edge",
"likelihood_offroad_indication",
]
df_scene_level["kinematic_metrics"] = df_scene_level[kinematic_cols].mean(axis=1)
df_scene_level["interactive_metrics"] = df_scene_level[interactive_cols].mean(axis=1)
df_scene_level["map_based_metrics"] = df_scene_level[map_cols].mean(axis=1) |
|
@daphne-cornelisse does this comment hold any water? |
riccardosavorgnan
force-pushed
the
ricky/merge_conflicts_3.0_beta_and_3.0
branch
from
b5adcd6 to
3464357
Compare
.github/workflows/perf-ci.yml
Outdated
| on: | ||
| pull_request: | ||
| branches: [ main ] | ||
| branches: [ 2.0 ] |
There was a problem hiding this comment.
these must be from the auto-merge, will fix
| """Evaluate a policy.""" | ||
|
|
||
| args = args or load_config(env_name) | ||
| args["env"]["termination_mode"] = 0 |
There was a problem hiding this comment.
what's this do? Magic number make me nervous
There was a problem hiding this comment.
termination mode 0 means we terminate the episode once all steps have been performed (I believe this is necessary for WOSAC). by contrast mode 1 means we terminate if all agents have been respawned at least once.
pufferlib/ocean/env_binding.h
Outdated
| c_get_road_edge_counts(drive, &np, &tp); | ||
| c_get_road_edge_polylines(drive, &x_base[pt_offset], &y_base[pt_offset], &lengths_base[poly_offset], | ||
| &scenario_ids_base[poly_offset]); | ||
| &scenario_ids_base[poly_offset * 16]); |
There was a problem hiding this comment.
It's because in WOMD the scenarios ids are a string of size 16
| wosac_target_scenarios = 64 | ||
| ; Total pool of scenarios to sample from | ||
| wosac_scenario_pool_size = 1000 | ||
| ; Max batches, used as a timeout to prevent an infinite loop |
There was a problem hiding this comment.
@WaelDLZ how are these two numbers different? I can see it in the code but it's unclear from the comment
There was a problem hiding this comment.
I think that in the long run these should be replaced by the multiprocessed eval code.
From my understanding:
- Pool size is the equivalent of the num_maps we would have in training.
- target_scenario is like stop the eval once we saw 64 distinct scenarios (this is needed because we sample with replacement)
- batch_size is we compute the metrics by batches of 32
But I hope that we can replace this by the mechanism I proposed in the multiprocessed eval PR
| human_replay_eval = False | ||
| ; Control only the self-driving car | ||
| human_replay_control_mode = "control_sdc_only" | ||
| ; Number of scenarios for human replay evaluation equals the number of agents |
| [traffic_light_violation] | ||
| bernoulli = true | ||
| metametric_weight = 0.05 | ||
| metametric_weight = 0.0 |
There was a problem hiding this comment.
@WaelDLZ why not set to correct value?
There was a problem hiding this comment.
Because we actually don't have a traffic light violation metric in our codebase. So to replicate the 2024 Wosac challenge we decided to do this
| // Skip if its the SDC | ||
| if (i == sdc_index) { | ||
| continue; | ||
| } |
pufferlib/ocean/drive/drive.h
Outdated
| char *scenario_id_ptr = scenario_ids_out + poly_idx * 16; | ||
| memcpy(scenario_id_ptr, env->scenario_id, 16); |
pufferlib/ocean/drive/drive.py
Outdated
| "y": np.zeros(total_points, dtype=np.float32), | ||
| "lengths": np.zeros(num_polylines, dtype=np.int32), | ||
| "scenario_id": np.zeros(num_polylines, dtype=np.int32), | ||
| "scenario_id": np.zeros(num_polylines, dtype="S16"), |
pufferlib/ocean/env_binding.h
Outdated
| c_get_global_ground_truth_trajectories( | ||
| drive, &x_base[traj_offset], &y_base[traj_offset], &z_base[traj_offset], &heading_base[traj_offset], | ||
| &valid_base[traj_offset], &id_base[agent_offset], &is_vehicle_base[agent_offset], | ||
| &is_track_to_predict_base[agent_offset], &scenario_id_base[agent_offset * 16]); |
- resolve conflicts on merge - adopt resample_maps() refactoring in drive.py - made the scenario_id string length attached to a constant.
riccardosavorgnan
force-pushed
the
ricky/merge_conflicts_3.0_beta_and_3.0
branch
from
aa91598 to
bca5dca
Compare
3 participants
mainto2.0#256)