Skip to content

fix: correct continuous-action PPO scaling, seeding, and eval defaults; retrain pinned models - #19

Open
aleksandarbabicdnv wants to merge 10 commits into
mainfrom
fix/ppo-acc-squared
Open

fix: correct continuous-action PPO scaling, seeding, and eval defaults; retrain pinned models#19
aleksandarbabicdnv wants to merge 10 commits into
mainfrom
fix/ppo-acc-squared

Conversation

@aleksandarbabicdnv

@aleksandarbabicdnv aleksandarbabicdnv commented Aug 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

The continuous-action PPO path had a dormant double-scaling bug that made the real
actuator 10× weaker than documented. Fixing it exposed a training pathology
(persistent bang-bang chatter) and, while investigating, two more issues: the
--seed CLI default was ignored, and play_ppo.py's --speed-sweep / --save-png
defaults silently did the wrong thing.

This PR fixes all of those, retrains the four pinned hybrid_cv01 baselines
(continuous + discrete, seeds 42 / 5775) so they are genuinely seeded and
reproducible, and rewrites docs/source/reward_comparison.md to match.

10 commits, ~7 source/script files, 4 retrained model sets, 1 doc rewritten.
Tests: 52 passed, 3 skipped (2 new environment tests).

Bugs fixed

Commit Problem Fix
bce6e90 reset() raised IndexError on any env whose first-ever reset lacked options={"init": True} — i.e. every parallel env in n_envs > 1, which is every real experiment config. Blocked all training. Guard on self.rewards being non-empty instead of the caller's options dict.
7411fdb step() continuous branch multiplied the (already physical-unit) action by conf.acc a second time. Real max acceleration was conf.acc² = 0.01, not 0.1. Remove the redundant * self.conf.acc. Regression test added.
c8973d8 Same double-scaling in ppo_agent.do_one_episode() → EpisodeResult.acc_final (used by play/sweep tables and plots). Remove the second rescale.
1ebf4cb At the corrected acc=0.1, the continuous policy converged to permanent bang-bang chatter: SB3's default initial policy σ (1.0) is ~10× the action-box half-width, so nearly every sampled action clips to a boundary before training shapes it. Auto-derive log_std_init = ln(acc / 2.5) in train_ppo.py (correct for any acc; explicit --log-std-init still overrides). Continuous agent now produces a smooth decaying correction and settles cleanly.
75552e8 --seed CLI default was hardcoded None instead of following --config, so every "pinned seed" run was actually unseeded (sidecars showed "seed": null). Default --seed from config.training.seed.
5ad9ae9 play_ppo.py: (1) --randomize-start defaulted to the sidecar value (true for all hybrid_cv01*), so a --speed-sweep without --no-randomize-start drew random speeds per row — silently optimistic numbers. (2) --save-png defaulted true but the env only records traces in render_mode == "plot", so --render-mode none --save-png wrote nothing with no warning. --speed-sweep now forces randomize_start=False (logged) unless --randomize-start is passed; --render-mode none is promoted to plot (file only) when --save-png is set, with a warning for play-back; --save-png defaults off for sweeps. No change for explicit --speed-sweep --no-randomize-start / --render-mode plot.

API / behaviour changes

  • TrainingConfig gains acc: float = 0.1 and log_std_init: float | None = None;
    train_ppo.py gains --acc and --log-std-init flags.
  • play_ppo.py reads acc from the training sidecar when reconstructing the env
    (old sidecars default to 0.1, backward compatible).
  • play_ppo.py sweep/save-png defaults as described above.

Model retrains + docs (f3e066b, 6b01e02, d2a2c7f)

All four pinned baselines retrained fresh, correctly seeded, evaluated with
--speed-sweep --no-randomize-start:

Model Crashes /100 Notes
hybrid_cv01_s42 (cont) 0 mean |x_pos| 0.39 cm, EV@3M 0.987
hybrid_cv01_s5775 (cont) 0 mean |x_pos| 0.66 cm, EV@3M 0.986
hybrid_cv01_disc_s42 0 90/100 machine-ε; 10 speeds settle to a ±0.1 m offset attractor
hybrid_cv01_disc_s5775 1 (isolated, 8.2 m/s) 99/100 machine-ε

docs/source/reward_comparison.md §4–§7 rewritten: the "machine-epsilon for all 100
speeds" claim softened to "90–99/100" with single-run caveats; EV comparison updated
(continuous now edges discrete); §6.1 footnote added for the discrete EV dip during
crash-elimination.

The seeded discrete runs are marginally less pristine than the previous committed
numbers, which were unreproducible: null seed and untracked *_vecnorm.pkl
(inference normalizes observations from it). 2215430 adds gitignore exceptions and
commits the four *_vecnorm.pkl, so all four pinned models are now genuinely
reproducible from their config + seed.

@aleksandarbabicdnv
fix(controlled_crane_pendulum): don't crash reset() when no episode h…
bce6e90 
…as run yet

reset() unconditionally called reward_stats_calc(len(self.rewards)) on
any non-"init" reset, assuming self.rewards was non-empty. That's true
for envs[0] (pre-reset by do_training() with options={"init": True}),
but every other parallel env in a vectorized training run (n_envs > 1,
as in every experiments/*.yaml) hits its first-ever reset with
self.rewards == [] and crashes with IndexError on rewards[-1].

Guard on self.rewards being non-empty instead of relying on the
caller having passed options={"init": True} on that specific
instance — this was blocking all real training runs with n_envs > 1,
independent of continuous vs. discrete actions.
@aleksandarbabicdnv
fix(controlled_crane_pendulum): remove double-scaling of continuous a…
7411fdb 
…cceleration

a028b57 changed the continuous action_space Box bounds from
Box(-1.0, 1.0) (normalized) to Box(-conf.acc, conf.acc) (physical
units), but step() still scaled the sampled action by conf.acc again,
capping the real applied acceleration at conf.acc ** 2 instead of
conf.acc. With the default acc=0.1, real max acceleration was 0.01 —
10x smaller than intended.

Add a regression test asserting step()'s applied acceleration matches
conf.acc at the action-space boundary.
@aleksandarbabicdnv
fix(ppo_agent): remove second acc-squared instance in do_one_episode()
c8973d8 
Same double-scaling bug as step()'s continuous branch, in a separate
diagnostic-only code path: EpisodeResult.acc_final (used by
play_ppo.py's play/sweep tables and plots) rescaled the already-
physical-units action by conf.acc again, showing acc**2 instead of
the real final acceleration. Found via the s42 speed-sweep output
showing x_acc_f pinned at exactly 0.01 (0.1**2) across nearly every
speed. Doesn't affect training or the environment's actual physics —
reporting only.
@aleksandarbabicdnv
fix(ppo): auto-size continuous policy's initial std from acc, fixing …
1ebf4cb 
…bang-bang chatter

Continuous-action PPO training at the physically-intended acc=0.1 was converging
to a persistent bang-bang chatter that never settled. Root cause: SB3's default
initial policy std (1.0) is far wider than the action box (half-width = acc) at
any acc value, so nearly every raw sampled action clips to the boundary before
training has shaped anything - a self-reinforcing near-deterministic bang-bang
distribution that gradient descent has no signal to escape from (log-prob is
computed from the raw unclipped sample, not the delivered clipped action).

Fix: expose `acc` as a proper TrainingConfig field (was previously hardcoded)
and auto-derive the continuous policy's initial log_std as log(acc / 2.5) in
train_ppo.py whenever it isn't set explicitly, so ~99% of raw samples start
inside the box regardless of acc - including if acc is swept/randomized across
future experiments, not just fixed at 0.1. Verified end-to-end: trained model
settles cleanly to rest with acc=0.1 (matching the Q-learning agent's action
range) instead of oscillating indefinitely, confirmed flat across a full
speed-sweep. See project_ppo_acc_squared_bug.md for the full investigation.

- experiment_config.py: TrainingConfig.acc and TrainingConfig.log_std_init
  fields, with from_dict wiring.
- ppo_agent.py: ProximalPolicyOptimizationAgent accepts log_std_init, passed to
  SB3 as policy_kwargs.
- train_ppo.py: --acc and --log-std-init CLI flags; auto-derivation logic.
- play_ppo.py: reads acc from the training sidecar so playback reconstructs the
  correct action-space bounds for models trained at a non-default acc.
@aleksandarbabicdnv
fix(train_ppo): wire --seed CLI default to config.training.seed
75552e8 
--seed defaulted to a hardcoded None instead of following --config like every
other training parameter (--acc, --ent-coef, --log-std-init, etc.), so a fresh
training run via `train_ppo.py --config <yaml>` alone silently ignored the
yaml's seed field and trained non-deterministically. Confirmed via training
sidecars: hybrid_cv01_s42.zip and hybrid_cv01_s5775.zip (and every other
fresh-trained model this session) all recorded seed: null despite their yamls
specifying seed: 42 / 5775 - none of the "pinned seed" reproducibility the
experiment yamls' own header comments promise was actually happening.
@aleksandarbabicdnv
chore(reward_comparison): retrain s42/s5775 continuous models, update…
f3e066b 
… doc

Retrains under the log_std_init auto-sizing fix (fixes the persistent bang-bang
chatter this branch's earlier commits addressed) and the --seed default fix
(models are now genuinely trained at seed 42/5775, not the previously-silent
seed=None). Discrete counterparts are untouched - never affected by either fix.

Evaluated with --no-randomize-start explicitly (the CLI default follows the
training config, which is randomize_start: true - omitting this flag draws a
random initial speed per sweep row instead of the requested fixed one, and
silently produced misleadingly optimistic sweep numbers during this retrain
investigation before the mistake was caught).

Regenerates all 8 continuous-side figures (2 training-curve comparisons, 2
sweep comparisons, 2 detail plots, 2 individual episode trajectories) and
rewrites reward_comparison.md's continuous-column numbers throughout.

Notable changes to the reported conclusions, not just numbers:
- Both seeds are now 100% crash-free across the full +-10 m/s sweep (previously
  s42 had 3 outlier speeds including an 835-step anomaly).
- Neither seed produces a value-function instability event during training
  (previously s5775 had one at 1.35M steps); section 4.4 rewritten accordingly.
- Settle step scales smoothly and roughly linearly with |speed| with no
  anomalous outliers in either seed - a cleaner result than previously
  reported, but not literally flat as an earlier draft of this update
  incorrectly claimed before the evaluation bug above was found.
- Continuous explained variance now exceeds discrete for both seeds (previously
  the reverse) - EV is no longer part of discretisation's advantage in S7.4,
  only position precision and settle speed are.
@aleksandarbabicdnv
chore(reward_comparison): retrain s42/s5775 discrete models, update doc
6b01e02 
Retrains under the --seed default fix (75552e8): the discrete baselines were
previously trained at seed=None despite their yaml "pinned seed" headers
(sidecars showed "seed": null). Now genuinely seed 42 / 5775.

Evaluated with --speed-sweep --no-randomize-start (the CLI default follows the
training config's randomize_start: true, which silently draws a random speed
per sweep row).

Also tracks <model>_vecnorm.pkl for the pinned discrete models: play_ppo.py
normalises observations from it at inference, so the models were not
reproducible without it regardless of the seed.

Notable result changes vs the previous (unseeded, unreproducible) numbers:
- disc s5775: 99/100 crash-free across the sweep (one isolated rail-contact
  transient at 8.2 m/s; adjacent speeds settle normally). EV 0.954 -> 0.971.
- disc s42: machine-epsilon final position for 90/100 swept speeds; ten
  converge to a +/-0.1 m offset attractor instead of exactly zero. Mean
  |x_pos| ~1.0 cm. EV 0.931 -> 0.928.
- Both single-run effects (one seed, one episode per speed); an earlier
  unseeded disc s42 run hit x=0 at every speed.
- reward_comparison.md sections 4-7 updated: "machine-epsilon for all 100
  speeds" softened throughout, single-run caveats added, figures regenerated.
- Fig 9 episode plot moved 1.0 -> 1.2 m/s (1.0 is now an offset speed).

No code changes.
@aleksandarbabicdnv
chore(repo): track continuous vecnorm stats; gitignore .claude/
2215430 
- Track models/hybrid_cv01_s42_vecnorm.pkl and hybrid_cv01_s5775_vecnorm.pkl
  so the pinned continuous models (f3e066b) are reproducible: play_ppo.py
  normalises observations from <stem>_vecnorm.pkl at inference. The discrete
  pair was already tracked in 6b01e02; this closes the same gap for continuous.
- Ignore .claude/ (Claude Code local session/config, not for the repo).
@aleksandarbabicdnv
fix(play_ppo): stop --speed-sweep and --save-png silently misbehaving
5ad9ae9 
Two CLI-default footguns found while re-evaluating models on this branch:

- --randomize-start defaulted to the model sidecar's value, which is
  randomize_start: true for every hybrid_cv01* config. A --speed-sweep run
  that didn't also pass --no-randomize-start therefore drew a random speed
  per row instead of the exact requested one, silently producing
  misleadingly optimistic sweep numbers. Now: default is a None sentinel,
  resolved to False for --speed-sweep (logged) and to the sidecar value
  otherwise; pass --randomize-start to force it back on for a sweep.

- --save-png defaulted to True but the env only records trajectory traces
  while render_mode == "plot" (AntiPendulumEnv.step), so --render-mode none
  --save-png wrote nothing, with no warning. Now: --render-mode none is
  promoted to "plot" (file only, no window) when --save-png is on; a
  warning is logged for --render-mode play-back, which can't save. Default
  is also off for --speed-sweep (which would otherwise emit one PNG per
  swept speed) and on for single-speed playback.

No behaviour change for existing explicit invocations
(--speed-sweep --no-randomize-start, --render-mode plot).
@aleksandarbabicdnv
docs(reward_comparison): explain the discrete EV dip in section 6.1
d2a2c7f 
The discrete expl_var curve in Figure 5 dips to ~0.04 around 800k-1.05M
steps before recovering to 0.93. Note that this coincides with discrete
crash-elimination (rail_hit% still falling over the same window) and is a
normal pre-convergence transient, not a value-function instability event -
EV is monotone non-decreasing from 1.05M onward for both variants.
@aleksandarbabicdnv
aleksandarbabicdnv requested review from ClaasRostock and eisDNV and removed request for ClaasRostock August 31, 2026 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eisDNV eisDNV Awaiting requested review from eisDNV

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@aleksandarbabicdnv