fix the burn through of goals at the beginning of the episode + test#503
Merged
Conversation
| @@ -0,0 +1,223 @@ | |||
| """Tests that replay-mode waypoint progression burns exactly ONE waypoint per | |||
There was a problem hiding this comment.
This comment should probably be compressed? For example, it contains the "how we got here" story
Comment on lines
+15
to
+26
| get_state() exposes, per agent: the goal alias (goal_position_x/y), the | ||
| per-step reached-goal flag (metrics_array[REACHED_GOAL_METRIC_IDX], recomputed | ||
| every step), and the full logged trajectory. The expected waypoint list is | ||
| reconstructed with the same sampling formula c_reset uses in replay mode, so | ||
| each observed alias maps to a definite waypoint index k, and "one waypoint per | ||
| step" is asserted as: every fire moves k by exactly +1 (or saturates on the | ||
| final waypoint), and k never moves without a fire. | ||
|
|
||
| Note: env construction and env.reset() each run compute_metrics once at reset | ||
| time, and on this fixture each of those calls legitimately consumes one | ||
| waypoint. The tests account for that via the waypoint index of the post-reset | ||
| alias instead of assuming the episode starts at waypoint 0. |
There was a problem hiding this comment.
why does the reader of this test file need to know this?
riccardosavorgnan
force-pushed
the
ricky/fix_replay_goal_burn_through
branch
from
fae026e to
a1302b6
Compare
Yvonne511
approved these changes
Jul 7, 2026
Yvonne511
left a comment
Yvonne511
left a comment
Collaborator
There was a problem hiding this comment.
This is perfectly good:
compute_metrics and current_goal_idx++ inside it could happen without step (in cases where reset is already on goal). Hence, current_goal_idx++ and update agent->current_goal should happen together. Update agent->current_goal should not be constrained inside step.
eugenevinitsky
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the burn through of waypoints at episode starts. The previous code burned through the goal at environment reset time, but advanced the alias of the actual goal only later in the step. This meant that if you spawn on a waypoint in replay mode, then consecutive waypoints would have been consumed because of the non updated "agent->goal_position_x" (which is only updated later in the c_step).
The test has been written with Claude and based on a map where we know the first waypoint is on the spawn point.