Sample DelayBuffer lags on the first step after creation or reset - #1203
Open
Zhuoxi2000 wants to merge 1 commit into
Open
Zhuoxi2000 wants to merge 1 commit into
Zhuoxi2000 wants to merge 1 commit into
Conversation
DelayBuffer started every row at lag 0, and reset() set it back to 0. The lag was only replaced on the row's next scheduled refresh, so with update_period > 0 and per_env_phase=True (the default) most rows kept lag 0 for up to update_period - 1 steps, and with hold_prob > 0 the first refresh could hold the 0. A min_lag=max_lag=2 buffer therefore returned the undelayed frame for many environments at the start of every episode, which breaks the documented [min_lag, max_lag] range for both observation and actuator delays. Rows that have no lag since creation or reset now always sample on their next compute(), regardless of update phase or hold_prob. A lag set explicitly with set_lags() is still kept. reset() still reports lag 0 until the next compute().
This branch has not been deployed
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.
DelayBufferstarts every row at lag 0, andreset()sets it back to 0. A row's lag only changes on that row's next scheduled refresh. Withupdate_period > 0andper_env_phase=True(the default), most rows therefore keep lag 0 for up toupdate_period - 1steps. Withhold_prob > 0, the first refresh can also keep the 0. As a result, even a constant-delay buffer (min_lag=max_lag=2) returns the undelayed frame for many environments at the start of every episode. This contradicts the documented contract:min_lagis "Minimum lag (inclusive)", lags are "sampled uniformly from [min_lag, max_lag]", and the docs say to use min=max for constant delay. It affects both observation terms and actuators that setdelay_*. With the example fromdocs/source/actuators.rst(min 2, max 5, hold 0.3, period 10, 8 envs, seed 0), 7 of 8 envs have lag 0 for the first 7 steps.This change adds a per-row flag for rows that have not had a lag sampled or set since creation or reset. Those rows always sample on their next
compute(), regardless of update phase orhold_prob, and then follow the normal schedule. A staggered env now samples at t=0 and then at its usual phase. A lag set withset_lags()before the first step is still kept, whichtest_delayed_actuator_set_lags_affects_delayrelies on.reset()still reports lag 0 until the nextcompute(), so the existing reset tests are unchanged. The change adds no random draws and no host sync.test_lags_stay_within_range_from_first_stepcovers the staggeredupdate_periodcase and thehold_probcase, both at creation and after a full reset. Both cases fail on main and pass with the fix. With the fix,tests/test_delay_buffer.py,test_delayed_actuator.py,test_observation_delay.py,test_actuator_builtin_group.py,test_observation_manager.pyandtest_actuator.pypass, and ruff, ty and pyright are clean.#1191 also touches
_sample_lags, but only the hold draw forper_env=False. This change merges cleanly with it apart from the changelog entry.AI assistance: this change was drafted with an AI coding assistant (Claude) and verified locally with the tests above.