diff --git a/tests/test_ddpg_integration.py b/tests/test_ddpg_integration.py index a55fdfe..54558cb 100644 --- a/tests/test_ddpg_integration.py +++ b/tests/test_ddpg_integration.py @@ -13,6 +13,7 @@ import numpy as np import pytest +import tensorflow as tf from alloc.models.networks import ActorCriticNetworks @@ -107,3 +108,109 @@ def test_training_step_reduces_critic_loss(networks: ActorCriticNetworks) -> Non # Soft check: the loss must not explode (DDPG is stochastic, so no strict # monotonic decrease is asserted). assert loss_after < 10.0 * max(loss_before, 1e-6) + + +# ===================================================================== +# TICKET-054: deeper DDPG coverage +# (a) multi-episode critic-loss trend +# (b) in-loop Polyak soft-update (targets track online networks) +# ===================================================================== + +N_EPISODES = 8 +STEPS_PER_EPISODE = 12 +BATCH_SIZE = 8 + + +def _run_closed_loop( + networks: ActorCriticNetworks, + seed: int = 42, + n_episodes: int = N_EPISODES, + steps_per_episode: int = STEPS_PER_EPISODE, + batch_size: int = BATCH_SIZE, +) -> list[float]: + """Run a deterministic closed-loop, multi-episode DDPG training loop. + + Each step samples an action through the public ``get_allocation`` API, + advances the toy environment, stores the transition, and — once the buffer + holds at least ``batch_size`` transitions — performs a critic update, an + actor update and a Polyak soft-update of the target networks. Returns the + per-update critic losses in order. + """ + rng = np.random.default_rng(seed) + losses: list[float] = [] + for _ in range(n_episodes): + state = rng.standard_normal(INPUT_DIM).astype(np.float64) + for _ in range(steps_per_episode): + action = networks.get_allocation(state) + next_state, reward = _env_step(state, action) + networks.replay_buffer.add(state, action, reward, next_state) + state = next_state + if len(networks.replay_buffer) >= batch_size: + s, a, r, ns = networks.replay_buffer.sample(batch_size) + losses.append(networks.update_critic(s, a, r, ns)) + networks.update_actor(s) + networks._soft_update_targets() + return losses + + +def test_multi_episode_critic_loss_trend(networks: ActorCriticNetworks) -> None: + """Over a closed-loop multi-episode loop the critic loss trends down. + + The existing ``test_training_step_reduces_critic_loss`` checks a fixed + random batch; this test checks the *closed-loop, multi-episode* dynamics + the DDPG Bellman target assumes: the critic should actually learn, so the + mean loss over the later third of updates is well below the earlier third. + """ + losses = _run_closed_loop(networks) + + # Enough updates to split into thirds. + assert len(losses) >= 9 + # Every critic loss is finite (no NaN/Inf anywhere in the loop). + assert all(math.isfinite(loss) for loss in losses) + + third = len(losses) // 3 + first_third = float(np.mean(losses[:third])) + last_third = float(np.mean(losses[-third:])) + + # The critic learns: the later-third mean loss is at least 25% below the + # earlier-third mean. (Measured ~57-68% reduction for seed=42; the 25% + # margin keeps the assertion robust to small numerical drift.) + assert last_third < 0.75 * first_third + + +def test_soft_update_targets_track_online_in_loop(networks: ActorCriticNetworks) -> None: + """Inside the closed-loop loop, Polyak targets track the online networks. + + ``_soft_update_targets`` (the real Polyak update used by the production + loop, ``core.py``) is covered in isolation by ``TestSoftUpdateTargets``; + this test verifies it in context: after training, the target weights are + close to — but not identical to — the online weights, and the target + critic's Q-estimate on a probe batch is close to the online critic's. + """ + _run_closed_loop(networks) + + # Target weights lag the online weights: close, but not identical. + for name in ("actor", "critic"): + online = getattr(networks, name).get_weights() + target = getattr(networks, f"{name}_target").get_weights() + max_diff = max( + float(np.max(np.abs(o - t))) for o, t in zip(online, target) + ) + # Not identical (training moved the online weights; targets lag). + assert max_diff > 0.0 + # Close (Polyak keeps targets near online; tau=0.005 is small). + assert max_diff < 0.5 + + # The target critic's Q-estimate on the same (state, action) probe is + # close to the online critic's (isolates critic-target tracking). + + rng = np.random.default_rng(0) + probe_s = rng.standard_normal((BATCH_SIZE, INPUT_DIM)).astype(np.float32) + probe_a = rng.random((BATCH_SIZE, NUM_ASSETS)).astype(np.float32) + q_online = tf.squeeze( + networks.critic([probe_s, probe_a], training=False), axis=-1 + ).numpy() + q_target = tf.squeeze( + networks.critic_target([probe_s, probe_a], training=False), axis=-1 + ).numpy() + assert float(np.max(np.abs(q_online - q_target))) < 1.0 diff --git a/tickets/TICKET-054.md b/tickets/TICKET-054.md new file mode 100644 index 0000000..82ed174 --- /dev/null +++ b/tickets/TICKET-054.md @@ -0,0 +1,62 @@ +# TICKET-054: Deeper DDPG coverage — multi-episode critic-loss trend + in-loop Polyak soft-update + +- **GitHub issue:** #122 +- **Target:** `tests/test_ddpg_integration.py` (add tests) +- **Status:** OPEN +- **Depends on:** TICKET-051 (closed-loop DDPG integration test, VERIFIED) + +## Evidence + +The DDPG closed-loop integration test (`tests/test_ddpg_integration.py`, 3 +tests) landed in Cycle 37 and is verified. Two genuine coverage gaps remain: + +1. **No multi-episode critic-loss trend assertion.** + - `test_closed_loop_training_step` (line 50) runs 16 closed-loop steps but + performs exactly **one** `update_critic` at the end — it asserts finiteness + only, never a loss *trend*. + - `test_training_step_reduces_critic_loss` (line 92) does 20 critic updates + but on a **fixed random batch** (not closed-loop episodes), so it does not + model the multi-episode dynamics the DDPG Bellman target assumes. + - There is no test that runs a *closed-loop, multi-episode* training loop and + asserts the critic loss trends (later-episode mean < earlier-episode mean). + +2. **No in-loop Polyak soft-update coverage.** + - `_soft_update_targets` (networks.py:463) is the real Polyak update used by + the production loop (`core.py:440`). `TestSoftUpdateTargets` + (test_actor_critic.py:421) covers the *formula* in isolation, but no test + verifies that, **inside a closed-loop training loop**, the target networks + track the online networks (targets stay close to, but not identical to, + online weights, and the target critic's Q-estimate stays near the online + critic's) as training progresses. + +## Impact + +- **False confidence on learning.** The integration suite proves the loop is + finite and the buffer overflows, but never that the critic actually *learns* + over multiple episodes (loss trending down). A regression that silently + disables learning (e.g. a broken Bellman target) would still pass. +- **Soft-update untested in context.** The Polyak update is the mechanism that + stabilises DDPG; testing it only in isolation misses interaction bugs with the + closed-loop dynamics (e.g. targets drifting away from online weights). + +## Minimal additive fix + +Add two tests to `tests/test_ddpg_integration.py` (no production-code change): + +1. `test_multi_episode_critic_loss_trend` — run a deterministic closed-loop, + multi-episode training loop (several episodes × several steps each, critic + update per step through the public `get_allocation` API), collect critic + losses, and assert: all finite, and the mean loss over the later half of + updates is strictly below the mean over the earlier half (learning is + happening). Fixed seed for reproducibility. + +2. `test_soft_update_targets_track_online_in_loop` — during the same closed-loop + training loop, after the Polyak soft-updates, assert the target actor/critic + weights are close to (but not identical to) the online weights, and the + target critic's Q-estimate on a probe batch is close to the online critic's + Q-estimate (Polyak keeps targets lagging but near). + +## Verification + +- New tests pass under `POLYGON_API_KEY=dummy pytest tests/test_ddpg_integration.py -q`. +- Full suite green at the Cycle 39 gate.