Skip to content

Fix TensorboardCallback logging for off-policy algorithms (DDPG/TD3/SAC)#1422

Open
jenish-25 wants to merge 1 commit into
AI4Finance-Foundation:masterfrom
jenish-25:fix/1395-tensorboard-off-policy-logging
Open

Fix TensorboardCallback logging for off-policy algorithms (DDPG/TD3/SAC)#1422
jenish-25 wants to merge 1 commit into
AI4Finance-Foundation:masterfrom
jenish-25:fix/1395-tensorboard-off-policy-logging

Conversation

@jenish-25

Copy link
Copy Markdown

Summary

Fixes #1395.

TensorboardCallback._on_rollout_end assumed the model always exposes a rollout_buffer, which only on-policy algorithms (A2C, PPO) have. Off-policy algorithms (DDPG, TD3, SAC) expose a replay_buffer instead, so the callback raised a KeyError on every rollout, fell into the except branch, logged None for train/reward_min|mean|max, and printed Logging Error: ... noise. Reward curves were effectively broken for off-policy training.

Fix

Read whichever buffer the model actually exposes and compute the reward statistics over the valid (filled) region:

buffer = getattr(self.model, "rollout_buffer", None) or getattr(self.model, "replay_buffer", None)
if buffer is None:
    return True
rewards = np.asarray(buffer.rewards)
if not getattr(buffer, "full", False):          # replay buffer isn't full early on
    rewards = rewards[: getattr(buffer, "pos", 0)]  # ignore the zero-initialized tail
rewards = rewards.flatten()
if rewards.size == 0:
    return True
self.logger.record(key="train/reward_min", value=float(rewards.min()))
self.logger.record(key="train/reward_mean", value=float(rewards.mean()))
self.logger.record(key="train/reward_max", value=float(rewards.max()))

This reads the buffer from self.model (stable across SB3 versions) rather than self.locals, so reward logging now works for both algorithm families. The import statistics it replaced is removed.

Tests

Added unit_tests/test_tensorboard_callback.py covering:

All four pass; black is clean on the changed files.

Note

This is a clean re-submission of the previously-closed #1404 (a maintainer asked for a fresh PR in that thread). The implementation reads the buffer off the model and adds the missing tests.

cc @bruceyang @spencerromo @xiaoyang

…dCallback

TensorboardCallback._on_rollout_end assumed the model always exposes a
rollout_buffer (A2C/PPO). Off-policy algorithms (DDPG, TD3, SAC) use a
replay_buffer instead, so the callback raised a KeyError every rollout,
logged None reward stats, and printed noisy errors.

Read whichever buffer the model exposes and compute reward min/mean/max
over the valid (filled) region, so reward logging works for both on- and
off-policy algorithms. Add unit tests covering both buffer types, the
partially-filled replay buffer, and the no-buffer case.

Fixes AI4Finance-Foundation#1395
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DDPG / off-policy algorithms fail due to rollout_buffer logging in FinRL 0.3.8

1 participant