fix: GAE ends an episode at the step that ended it, not one later - #86
Merged
Merged
Conversation
Both servers call feedback for step t with terminated/truncated set to step t-1's outcome and next_terminated/next_truncated to step t's, and chain an episode's first frame to the last frame of the episode before. A frame carrying `terminated` therefore begins an episode. Until 48042e5 the GAE recursion cut a linked frame with the successor's dones[next_idx] - step t's own outcome. 48042e5 made it read the frame's own dones[step] (terminated[step]/truncated[step]) - step t-1's - so since then the step that ended an episode bootstrapped from the next episode's first state and kept accumulating its advantages, and each episode's first step was cut off from the rest. It also made a chain end carrying `dones` count as terminal and drop its bootstrap value. Both branches now read whether a frame's own transition ended from next_*. Three 3-step episodes of reward 1 at zero value, gamma = lambda = 1: main gave 4 3 2 1 3 2 1 2 1; the answer is 3 2 1 three times.
This was referenced Sep 27, 2026
Merged
…e bit The factors are powers of two, but Adam's eps is added unscaled and leaves a trace in the last bits of elements whose gradient is within a few orders of it. After this branch changed the toy's returns, one parameter came out a few bits apart on CI and identical on Windows and on a Linux workstation, all torch 2.7.1.
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
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.
GAE has been ending every episode one step late since 48042e5 (2026-04-03). FPO, DPPO and anything else built on
GAEBufferhave been affected.The calling convention. Both servers call
feedbackfor step t with:terminated/truncated= step t−1's outcomenext_terminated/next_truncated= step t's outcomeprev_node= step t−1's nodeprev_nodeis never reset at an episode boundary. So a frame carryingterminatedis the first frame of a new episode, and it is chained to the last frame of the episode before.What went wrong. Before 48042e5,
compute_advantages_and_returnscut the recursion at a linked frame with the successor'sdones[next_idx]. Under this convention that is step t's own outcome, which is correct. 48042e5 switched to the frame's owndones[step](andterminated[step]/truncated[step]), which is step t−1's outcome. Since then:The same commit also made a chain end that carries
dones(a frame that begins an episode and then meets the end of the buffer) count as terminal, so it dropped its bootstrap value.Worked by hand: one env, reward 1, values 0, γ = λ = 1, three 3-step episodes.
The fix. Whether a frame's own transition ended its episode is now read from
next_*in both branches. For linked frames this is what the code did before 48042e5. For chain ends it drops the extra sub-branch. The masking of a truncated step whentreat_truncated_as_doneis off is unchanged.tests/test_gae_episode_boundary.pyfeeds the buffer exactly as the servers do and checks against advantages worked by hand. It covers:treat_truncated_as_donesettingsAll six fail on main. The full suite passes (242 passed, 3 skipped).
One side effect showed up in the suite.
test_fpo_holds_what_it_learnsis FPO's known-defect test, marked xfail. It checks that FPO keeps what it learned on the bandit, whose episodes are three steps long. On this machine it held in 4 of its 10 cases before the fix and holds in 7 after. The test stays marked xfail.What this does to earlier results. Every experiment since April ran with this defect. The coverage figure's green cells learned in spite of it. Their numbers were measured on the defective code and stay as measured. E36 and E37, both running now, are also on it. HalfCheetah never terminates, so it is affected only at its 1000-step truncations. Hopper and Walker2d, which end on a fall, are affected at every fall.
What it does on a real task. This is a diagnostic, not a registered experiment:
gaussian-policyunderppo(#85) on Hopper-v5, 100 iterations of 2,048 steps, seeds 0–2. Both arms ran the same code except for this fix. The runs are ab6e32f and 274929c on guangzhao. Mean return:With the fix, learning was faster early on and the three seeds were closer together. By iteration 100 the two arms cannot be told apart on three seeds. The defect is established by the tests above, not by this table, and on this task its cost was modest.
One test tightened its claim.
test_scaling_the_value_loss_changes_nothingasserted that value-loss coefficients 0.25, 1.0 and 4.0 give bit-identical weights. After the fix changed the toy's returns, one parameter came out a few bits apart on CI, while it stayed identical on Windows and on a Linux workstation (all torch 2.7.1). The factors are powers of two, so the scaled gradients and moments are exact. The difference comes from Adam'seps, which is added unscaled. The test now asserts equality to float32 rounding, and the docstring says why. Any real effect of the coefficient would still fail it by orders of magnitude.