From e3c3cf0fa3a80552c90f57727c21554b2bbdffc4 Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Fri, 6 Mar 2026 14:32:37 +0100 Subject: [PATCH] docs: add DQN hyperopt overhaul implementation plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 7 tasks covering: 5-action compatibility in eval/backtest paths, C2 triple exploration stacking fix (remove count bonus from Q-values, narrow epsilon floor, reduce search space 30D→29D), PPO isolation verification. Co-Authored-By: Claude Opus 4.6 --- ...06-dqn-hyperopt-overhaul-implementation.md | 428 ++++++++++++++++++ 1 file changed, 428 insertions(+) create mode 100644 docs/plans/2026-03-06-dqn-hyperopt-overhaul-implementation.md diff --git a/docs/plans/2026-03-06-dqn-hyperopt-overhaul-implementation.md b/docs/plans/2026-03-06-dqn-hyperopt-overhaul-implementation.md new file mode 100644 index 000000000..78d642e30 --- /dev/null +++ b/docs/plans/2026-03-06-dqn-hyperopt-overhaul-implementation.md @@ -0,0 +1,428 @@ +# DQN Hyperopt Overhaul Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Fix remaining hyperopt-to-eval mismatches so hyperopt configs produce consistent out-of-sample results. + +**Architecture:** Three workstreams: (1) fix 5-action compatibility bugs in eval/backtest paths introduced by the action-diversity work, (2) remove count bonus from Q-value computation to eliminate triple exploration stacking, (3) simplify epsilon to noisy nets only during training. + +**Tech Stack:** Rust, Candle ML, DQN, factored action space (5 exposure levels) + +**Pre-Implementation Audit:** B1 (softmax eval), B2 (NormStats), B3 (portfolio sync), C1 (extrinsic-only replay), C3 (neutral hold), C4 (Sharpe stopping) are already implemented in prior commits. This plan covers the remaining gaps. + +--- + +## Task 1: Fix `FactoredAction::from_index()` in Hyperopt Backtest + +**Root Cause:** `hyperopt/adapters/dqn.rs:2940` calls `FactoredAction::from_index(action_idx)` with DQN output indices 0-4. Since the 45-action mapping has `exposure_idx = idx / 9`, indices 0-4 ALL map to `Short100` with different order/urgency combos. The hyperopt backtest silently runs every trial all-short. + +**Files:** +- Modify: `crates/ml/src/hyperopt/adapters/dqn.rs` + +**Step 1: Write the failing test** + +Add test in the adapter's test module that verifies DQN exposure indices 0-4 produce distinct exposure levels in the backtest path: + +```rust +#[test] +fn test_exposure_index_maps_to_correct_action() { + use crate::dqn::action_space::ExposureLevel; + use crate::dqn::order_router::OrderRouter; + + // DQN outputs 0-4 (exposure indices). Each must map to a distinct exposure. + let exposures: Vec = (0..5) + .map(|idx| ExposureLevel::from_index(idx).unwrap()) + .collect(); + + assert_eq!(exposures[0], ExposureLevel::Short100); + assert_eq!(exposures[1], ExposureLevel::Short50); + assert_eq!(exposures[2], ExposureLevel::Flat); + assert_eq!(exposures[3], ExposureLevel::Long50); + assert_eq!(exposures[4], ExposureLevel::Long100); + + // All must produce valid FactoredActions + for idx in 0..5 { + let exposure = ExposureLevel::from_index(idx).unwrap(); + let action = OrderRouter::route_default(exposure); + assert_eq!(action.exposure, exposure); + } +} +``` + +**Step 2: Run test to verify it passes (sanity check on ExposureLevel)** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib test_exposure_index_maps_to_correct_action -- --exact` +Expected: PASS (ExposureLevel::from_index already works correctly) + +**Step 3: Fix the backtest action conversion** + +In `crates/ml/src/hyperopt/adapters/dqn.rs`, line ~2940, replace: +```rust +let factored = crate::dqn::FactoredAction::from_index(action_idx)?; +``` +with: +```rust +let exposure = crate::dqn::action_space::ExposureLevel::from_index(action_idx)?; +let factored = crate::dqn::OrderRouter::route_default(exposure); +``` + +**Step 4: Fix unique_actions logging** + +Line ~2985: Change `unique_actions={}/{}, 45` to `unique_actions={}/{}, 5` + +**Step 5: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib -- hyperopt::adapters::dqn` +Expected: All hyperopt DQN adapter tests pass + +**Step 6: Commit** + +```bash +git add crates/ml/src/hyperopt/adapters/dqn.rs +git commit -m "fix(ml): hyperopt backtest uses ExposureLevel, not FactoredAction::from_index + +FactoredAction::from_index(0-4) maps all to Short100. ExposureLevel::from_index(0-4) +correctly maps to 5 distinct exposure levels." +``` + +--- + +## Task 2: Fix `FactoredAction::from_index()` in evaluate_baseline + +**Root Cause:** Same bug as Task 1 but in the standalone evaluation binary. `evaluate_baseline.rs:466` calls `FactoredAction::from_index(action_idx)` with DQN indices 0-4, producing all-Short100 evaluations. + +**Files:** +- Modify: `crates/ml/examples/evaluate_baseline.rs` + +**Step 1: Fix the CLI default** + +Line ~97: Change `default_value_t = 45` to `default_value_t = 5` for `num_actions`. Update the doc comment to reflect DQN uses 5 exposure levels. + +Note: PPO callers must now explicitly pass `--num-actions 45`. Add a note to the `--help` text. + +**Step 2: Fix simulate_chunk_trades action conversion** + +Line ~466: Replace: +```rust +let action = match FactoredAction::from_index(action_idx) { + Ok(fa) => fa, + Err(e) => { + warn!(" [{}] from_index({}) error at bar {}: {}", model_name, action_idx, bar_idx, e); + FactoredAction::new(ExposureLevel::Flat, ActionOrderType::Market, Urgency::Normal) + } +}; +``` +with: +```rust +let action = match ExposureLevel::from_index(action_idx) { + Ok(exposure) => ml::dqn::OrderRouter::route_default(exposure), + Err(e) => { + warn!(" [{}] exposure_from_index({}) error at bar {}: {}", model_name, action_idx, bar_idx, e); + FactoredAction::new(ExposureLevel::Flat, ActionOrderType::Market, Urgency::Normal) + } +}; +``` + +**IMPORTANT**: Check if `simulate_chunk_trades` is also called from the PPO eval path. If PPO calls it with indices 0-44, this function needs a `model_type` parameter to dispatch correctly. Read the PPO eval section (~lines 774-828) to confirm. + +**Step 3: Fix DQN config in eval** + +Line ~580: Verify `DQNConfig { num_actions: args.num_actions }` — with the default changed to 5, this should now be correct. But the PPO path must NOT use this config. + +**Step 4: Build check** + +Run: `SQLX_OFFLINE=true cargo check --workspace` +Expected: 0 errors, 0 warnings + +**Step 5: Commit** + +```bash +git add crates/ml/examples/evaluate_baseline.rs +git commit -m "fix(ml): evaluate_baseline uses ExposureLevel for DQN 5-action space + +DQN default num_actions=5. FactoredAction::from_index() replaced with +ExposureLevel::from_index() + OrderRouter. PPO must pass --num-actions 45." +``` + +--- + +## Task 3: Remove Count Bonus from Q-Value Computation (C2a) + +**Root Cause:** Count bonus (`broadcast_add` of UCB bonuses to Q-values before argmax) creates a second exploration mechanism on top of noisy nets + epsilon floor. This triple stacking produces overly exploratory training behavior, inflating trade counts in hyperopt trials (58K trades in 3 months). + +**Files:** +- Modify: `crates/ml/src/trainers/dqn/trainer.rs` + +**Step 1: Remove count bonus from `select_actions_batch()`** + +Lines ~3292-3307: Replace the count bonus block: +```rust +// Apply count bonus (UCB exploration) to Q-values before argmax. +// Previously only in DQN::select_action() (single-sample path), +// making the batch training path have no UCB exploration. +let use_cb = agent.use_count_bonus(); +let n_actions = agent.num_actions(); +let batch_q_values = if use_cb { + let bonuses = agent.get_count_bonuses(); + let bonus_tensor = Tensor::from_vec(bonuses, (1, n_actions), batch_q_values.device()) + .map_err(|e| anyhow::anyhow!("Failed to create bonus tensor: {}", e))? + .to_dtype(batch_q_values.dtype()) + .map_err(|e| anyhow::anyhow!("Failed to cast bonus tensor: {}", e))?; + batch_q_values.broadcast_add(&bonus_tensor) + .map_err(|e| anyhow::anyhow!("Failed to add count bonus: {}", e))? +} else { + batch_q_values +}; +``` +with: +```rust +// C2 FIX: Count bonus removed from Q-value computation. +// Noisy nets are the sole exploration mechanism during training. +// Count bonus kept for diversity metrics only (record_action tracking). +``` + +**Step 2: Remove count bonus from `select_actions_batch_gpu()`** + +Lines ~3367-3381: Same removal as Step 1. Replace the identical count bonus block with the same comment. + +**Step 3: Keep `record_action()` calls intact** + +Verify that `agent.record_action()` / `count_bonus.record_action()` calls remain in place after action selection. These track diversity metrics but no longer affect Q-values. Do NOT remove: +- `trainer.rs` calls to `reset_count_bonus()` at epoch boundary (line ~2717) +- Any `record_action()` calls post action selection + +**Step 4: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` +Expected: All 2728+ tests pass + +**Step 5: Commit** + +```bash +git add crates/ml/src/trainers/dqn/trainer.rs +git commit -m "fix(ml): remove count bonus from Q-value computation (C2) + +Noisy nets are sole exploration mechanism during training. +Count bonus UCB removed from argmax input in both batch paths. +Count bonus module kept for diversity metrics tracking only." +``` + +--- + +## Task 4: Remove Epsilon Floor from Training (C2b) + +**Root Cause:** `noisy_epsilon_floor` (default 0.05 = 5%) adds random epsilon-greedy exploration on top of noisy nets. With noisy nets providing learned, state-dependent exploration and only 5 actions, the epsilon floor is unnecessary noise. + +**Files:** +- Modify: `crates/ml/src/hyperopt/adapters/dqn.rs` + +**Step 1: Set noisy_epsilon_floor default to 0.0** + +Line ~465: Change: +```rust +noisy_epsilon_floor: 0.05, // Minimum random exploration with noisy nets +``` +to: +```rust +noisy_epsilon_floor: 0.0, // C2: Noisy nets provide exploration; no epsilon floor during training +``` + +**Step 2: Narrow the search space bounds** + +Line ~518: Change: +```rust +(0.02, 0.10), // 23: noisy_epsilon_floor (linear) +``` +to: +```rust +(0.0, 0.05), // 23: noisy_epsilon_floor (linear, 0.0=noisy-nets-only) +``` + +This allows TPE to discover if any epsilon floor helps, but biases toward 0. The lower bound 0.0 lets TPE select pure noisy-net exploration. + +**Step 3: Update clamp bounds** + +Line ~583: Change: +```rust +let noisy_epsilon_floor = x[23].clamp(0.02, 0.10); +``` +to: +```rust +let noisy_epsilon_floor = x[23].clamp(0.0, 0.05); +``` + +**Step 4: Update test assertions** + +Update test bounds assertions (~line 3620): +```rust +assert_eq!(bounds[23], (0.0, 0.05)); // noisy_epsilon_floor +``` + +Update test default value assertions and from_continuous test vectors for index 23. + +**Step 5: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib -- hyperopt::adapters::dqn` +Expected: All pass after assertion updates + +**Step 6: Commit** + +```bash +git add crates/ml/src/hyperopt/adapters/dqn.rs +git commit -m "fix(ml): narrow noisy_epsilon_floor to [0.0, 0.05] (C2) + +Default 0.0 = noisy nets only. TPE can still discover small epsilon +if beneficial, but no longer forced to 5-10% random exploration." +``` + +--- + +## Task 5: Remove count_bonus_coefficient from Hyperopt Search Space (C2c) + +**Root Cause:** Count bonus no longer modifies Q-values (Task 3). The coefficient is still in the 30D search space, wasting a dimension that TPE must explore. Removing it reduces 30D → 29D. + +**Files:** +- Modify: `crates/ml/src/hyperopt/adapters/dqn.rs` + +**Step 1: Analyze the impact** + +Before changing, verify: +1. `count_bonus_coefficient` is at index 29 (last dimension) +2. Removing it shifts NO other indices (it's last) +3. All `from_continuous()` / `to_continuous()` / `continuous_bounds()` / `param_names()` must be updated +4. Test vectors in test module must be updated + +**Step 2: Remove dimension 29** + +In `continuous_bounds()`: Remove `(0.05, 1.0), // 29: count_bonus_coefficient` entry. + +In `from_continuous()`: +- Change length check from 30 to 29 +- Remove `let count_bonus_coefficient = x[29].clamp(0.05, 1.0);` +- Hardcode: `count_bonus_coefficient: 0.1` (fixed default, only used for diversity metrics) + +In `to_continuous()`: +- Remove `self.count_bonus_coefficient, // 29` +- Output Vec shrinks from 30 to 29 + +In `param_names()`: +- Remove `"count_bonus_coefficient", // 29` + +**Step 3: Update all comments** + +Change "30D" → "29D" in all comments throughout the file. + +**Step 4: Update all tests** + +- Bounds test: `assert_eq!(bounds.len(), 29)` +- Names test: `assert_eq!(names.len(), 29)` +- from_continuous vectors: reduce from 30 to 29 elements +- Roundtrip test: verify count_bonus_coefficient is fixed at 0.1 + +**Step 5: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib -- hyperopt::adapters::dqn` +Expected: All pass after updates + +**Step 6: Commit** + +```bash +git add crates/ml/src/hyperopt/adapters/dqn.rs +git commit -m "fix(ml): remove count_bonus_coefficient from search space (29D) + +Count bonus no longer modifies Q-values (C2). Coefficient fixed at 0.1 +for diversity metrics only. Search space reduced from 30D to 29D." +``` + +--- + +## Task 6: Verify PPO Isolation + +**Root Cause:** PPO must remain at 45 actions. Verify no PPO paths were broken by the changes. + +**Files:** +- Read: `crates/ml/examples/evaluate_baseline.rs` (PPO eval section) +- Read: `crates/ml/src/cuda_pipeline/ppo_experience_kernel.cu` + +**Step 1: Verify PPO eval path uses 45 actions** + +Read evaluate_baseline.rs lines ~774-828 (PPO section). Confirm: +- PPO config uses `num_actions: 45` (or reads from checkpoint) +- PPO action indices 0-44 are correctly handled +- If PPO shares `simulate_chunk_trades()`, confirm it receives 0-44 indices and the function handles both DQN (0-4) and PPO (0-44) correctly + +If `simulate_chunk_trades()` is shared: add a parameter to dispatch between `ExposureLevel::from_index()` (DQN) and `FactoredAction::from_index()` (PPO). + +**Step 2: Verify CUDA PPO kernel** + +Confirm `ppo_experience_kernel.cu` still uses `PPO_NUM_ACTIONS=45` and `ppo_action_to_exposure()`. + +**Step 3: Run PPO-specific tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib -- ppo` +Expected: All PPO tests pass + +**Step 4: Run full workspace build** + +Run: `SQLX_OFFLINE=true cargo check --workspace` +Expected: 0 errors, 0 warnings + +**Step 5: Commit (if changes needed)** + +Only commit if PPO isolation required code changes. + +--- + +## Task 7: Full Test Suite + Clippy Verification + +**Files:** None (verification only) + +**Step 1: Full test suite** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` +Expected: 2728+ passed, 0 failed + +**Step 2: Clippy** + +Run: `SQLX_OFFLINE=true cargo clippy --workspace -- -D warnings` +Expected: 0 errors, 0 warnings + +**Step 3: Final commit** + +```bash +git add -A +git commit -m "chore(ml): DQN hyperopt overhaul complete + +B1-B3 (eval alignment): verified already implemented. +C1 (extrinsic-only replay): verified already implemented. +C2 (single exploration): count bonus removed from Q-values, + epsilon floor narrowed to [0.0, 0.05], search space 29D. +C3 (neutral hold): verified already implemented. +C4 (Sharpe stopping): verified already implemented. +5-action compatibility: fixed in hyperopt backtest + evaluate_baseline." +``` + +--- + +## Verification Checklist + +After all tasks: + +| Fix | Verification | +|-----|-------------| +| 5-action eval | `evaluate_baseline --model dqn` produces 5 distinct exposure actions | +| 5-action hyperopt backtest | Hyperopt trial backtest shows unique_actions across all 5 exposures | +| C2 count bonus | Q-values in batch paths have no UCB bonus added | +| C2 epsilon floor | `noisy_epsilon_floor` defaults to 0.0, range [0.0, 0.05] | +| C2 search space | 29D (count_bonus_coefficient removed) | +| PPO isolation | PPO tests pass, PPO still uses 45 actions | +| No regressions | 2728+ tests, 0 clippy warnings | + +## Risk Assessment + +| Risk | Mitigation | +|------|-----------| +| Removing count bonus reduces diversity | 5-action space is small enough for noisy nets; diversity penalty in objective still catches collapse | +| Removing epsilon floor causes exploitation collapse | Noisy nets provide state-dependent exploration; TPE can still select small epsilon if needed (range [0.0, 0.05]) | +| evaluate_baseline shared between DQN/PPO | Task 6 verifies PPO isolation; may need model_type dispatch in simulate_chunk_trades | +| 29D search space invalidates prior hyperopt results | Expected — prior results used degenerate 45-action space anyway |