Skip to content

Commit a61d4b2

Browse files
Joshua Bloomclaude
andcommitted
fix(actor): address PR #22 review — TD3 signature, tuple skip, docs, reset test
- learn_TD3 now accepts recurrent=False to match the DDPG/RDDPG signatures; the learn_gsp dispatch was passing 3 positional args to a 2-arg method. Latent bug today (GSP networks are built as DDPG/attention, not TD3) but removes the footgun before the diagnostic batch exercises TD3 variants. - TD3's non-actor-update step returns (0, 0); previously we unwrapped to 0.0 and logged it. That produces false collapse signals every update_actor_iter-1 ticks. Now we skip the entry entirely — leave last_gsp_loss at None as if no GSP step ran. - Doc the semantic: last_gsp_loss is the GSP learner's training loss, which is actor loss (policy-gradient signal) for DDPG/RDDPG/TD3 and genuine MSE only for attention. For prediction-collapse detection consumers should rely on gsp_squared_error and the HDF5Logger episode-level gsp_output_std / gsp_pred_target_corr attrs. - Add reset-between-ticks test covering the load-bearing invariant that last_gsp_loss returns to None when a learn() call runs but no GSP learning step fires. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 9b38f30 commit a61d4b2

3 files changed

Lines changed: 39 additions & 4 deletions

File tree

gsp_rl/src/actors/actor.py

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,16 @@ def __init__(
113113
self.build_gsp_network('attention')
114114
self.build_gsp_network('DDPG')
115115

116-
# Information-collapse diagnostic: last GSP prediction network training loss.
116+
# Information-collapse diagnostic: last GSP learner training loss.
117+
# NOTE: this is the loss returned by the GSP learner's inner learn step, which means:
118+
# - For DDPG/RDDPG/TD3 GSP schemes: actor loss (a critic-derived policy-gradient
119+
# signal), NOT the prediction MSE against delta-theta. A collapsed predictor may
120+
# not produce an anomalous value here, since the critic's value landscape can
121+
# support multiple policy solutions.
122+
# - For the attention GSP scheme: genuine prediction MSE against the label.
123+
# For prediction-collapse detection, prefer the raw per-step squared error captured
124+
# in RL-CollectiveTransport as `gsp_squared_error` plus the episode-level
125+
# `gsp_output_std` / `gsp_pred_target_corr` attrs computed in the Stelaris HDF5Logger.
117126
# Populated by learn_gsp() whenever a GSP learning step fires; reset to None at the
118127
# start of each learn() call so callers can distinguish "no GSP step this tick" from
119128
# "GSP step ran".
@@ -452,9 +461,12 @@ def learn_gsp(self):
452461
elif self.gsp_networks['learning_scheme'] == 'attention':
453462
loss = self.learn_attention(self.gsp_networks)
454463
if loss is not None:
455-
# TD3's edge-case path returns (0, 0); normalize to a scalar for logging.
464+
# TD3's non-actor-update steps return (0, 0) (critic stepped, actor did not).
465+
# Recording a legitimate 0.0 there would produce false collapse signals every
466+
# `update_actor_iter - 1` ticks, so skip those entries entirely — leave
467+
# last_gsp_loss at None as if no GSP step ran this tick.
456468
if isinstance(loss, tuple):
457-
loss = loss[0]
469+
return
458470
self.last_gsp_loss = float(loss)
459471

460472
def store_agent_transition(self, s, a, r, s_, d):

gsp_rl/src/actors/learning_aids.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,7 @@ def learn_RDDPG(self, networks, gsp = False, recurrent = False):
405405

406406
return actor_loss.item()
407407

408-
def learn_TD3(self, networks, gsp = False):
408+
def learn_TD3(self, networks, gsp = False, recurrent = False):
409409
states, actions, rewards, states_, dones = self.sample_memory(networks)
410410

411411
with T.no_grad():

tests/test_actor/test_gsp_loss_exposure.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,29 @@ def test_last_gsp_loss_populated_after_learn_with_gsp(self):
8585
assert actor.last_gsp_loss is not None
8686
assert isinstance(actor.last_gsp_loss, float)
8787

88+
def test_last_gsp_loss_resets_between_ticks(self):
89+
"""Each learn() call starts by resetting last_gsp_loss to None.
90+
91+
This is the load-bearing invariant of the field: consumers must be able to read
92+
it after learn() and distinguish "no GSP step ran this tick" (None) from
93+
"GSP step ran and returned a value" (float). If the reset fails, a stale value
94+
from a previous tick bleeds into the current tick's reading.
95+
"""
96+
actor = make_gsp_actor()
97+
fill_primary_and_gsp_buffers(actor)
98+
actor.learn()
99+
assert actor.last_gsp_loss is not None # populated after first learn
100+
101+
# Drain the GSP replay buffer below the batch size so the next learn_gsp early-returns.
102+
# Simulate by swapping in an empty gsp replay buffer. This is a white-box probe of the
103+
# reset invariant rather than a full end-to-end run.
104+
actor.gsp_networks['replay'].mem_ctr = 0
105+
106+
actor.learn()
107+
assert actor.last_gsp_loss is None, (
108+
"last_gsp_loss should reset to None when learn() runs but no GSP step fires"
109+
)
110+
88111
def test_last_gsp_loss_remains_none_when_gsp_disabled(self):
89112
"""Non-GSP actor never populates last_gsp_loss."""
90113
actor = Actor(

0 commit comments

Comments
 (0)