docs: trade lifecycle fix plan — address review feedback

- Fixed struct name: ExperienceCollectorConfig (not GpuExperienceCollectorConfig) - Fixed max_pos scoping: moved declaration before action_select launch - Fixed exiting_trade ordering: preliminary variable before trailing stop - Documented hardcoded 5-action limitation in branching_action_select - Zero Q-gap during hold periods (conviction meaningless when forced) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-03-25 23:02:39 +01:00
parent 2197db8472
commit d71bf5805e
1 changed files with 501 additions and 0 deletions
--- a/docs/superpowers/plans/2026-03-25-trade-lifecycle-fix.md
+++ b/docs/superpowers/plans/2026-03-25-trade-lifecycle-fix.md
@@ -0,0 +1,501 @@
+# Trade Lifecycle Fix — Implementation Plan
+
+> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
+
+**Goal:** Fix 4 trade lifecycle bugs that cause 77% turnover (620K trades / 800K bars) by adding 3 layers of defense: action masking, kernel hold override, and reward scaling. Target: <120K trades/epoch with min_hold_bars=5.
+
+**Architecture:** Layer 1 (action masking in `experience_action_select`) prevents the agent from choosing exit/reversal during hold period. Layer 2 (kernel override in `experience_env_step`) catches any action that slips through masking. Layer 3 (reward scaling) gives smooth gradient toward longer holds. Config wires `min_hold_bars` through DQNHyperparameters → TOML → hyperopt.
+
+**Tech Stack:** CUDA kernels (C), Rust (cudarc 0.19.3), TOML config.
+
+**Critical path:** Only `experience_action_select` and `experience_env_step` in `experience_kernels.cu` are in the GPU-fused training loop. The `branching_action_select` in `epsilon_greedy_kernel.cu` is only used by the backtest evaluator and tests — not the training hot path.
+
+---
+
+## Task 1: Add `min_hold_bars` to config and wire through TOML
+
+**Files:**
+- Modify: `crates/ml/src/trainers/dqn/config.rs`
+- Modify: `crates/ml/src/training_profile.rs`
+- Modify: `config/training/dqn-smoketest.toml`
+- Modify: `config/training/dqn-production.toml`
+
+- [ ] **Step 1: Add `min_hold_bars` field to `DQNHyperparameters`**
+
+In `config.rs`, after the `bars_per_day` field (~line 905), add:
+
+```rust
+    /// Minimum bars to hold a position before allowing exit or reversal.
+    /// Prevents per-bar churning. Trailing stop can override after this period.
+    /// Default: 5 (about 5 minutes for 1-min bars). Range: [2, 20].
+    pub min_hold_bars: usize,
+```
+
+- [ ] **Step 2: Set default in `conservative()` and `Default` impl**
+
+In `conservative()` (~line 1376 area, after `bars_per_day`):
+```rust
+            min_hold_bars: 5,
+```
+
+- [ ] **Step 3: Add to TOML experience section in `training_profile.rs`**
+
+Find the `ExperienceSection` struct (or the section that handles `[experience]` TOML). Add:
+```rust
+    pub min_hold_bars: Option<usize>,
+```
+
+In the `apply_to()` method:
+```rust
+    if let Some(v) = self.min_hold_bars {
+        hp.min_hold_bars = v;
+    }
+```
+
+- [ ] **Step 4: Update TOML files**
+
+In `config/training/dqn-smoketest.toml`, add under `[experience]`:
+```toml
+min_hold_bars = 3
+```
+
+In `config/training/dqn-production.toml`, add under `[experience]`:
+```toml
+min_hold_bars = 5
+```
+
+- [ ] **Step 5: Build + test**
+
+Run: `SQLX_OFFLINE=true cargo check -p ml --lib`
+Run: `SQLX_OFFLINE=true cargo test -p ml --lib -- training_profile`
+Expected: compiles, profile tests pass
+
+- [ ] **Step 6: Commit**
+
+```bash
+git add crates/ml/src/trainers/dqn/config.rs crates/ml/src/training_profile.rs config/training/dqn-smoketest.toml config/training/dqn-production.toml
+git commit -m "feat: add min_hold_bars config (default 5) — prevents per-bar churning"
+```
+
+---
+
+## Task 2: Layer 2 — Fix hold enforcement in `experience_env_step` kernel
+
+**Files:**
+- Modify: `crates/ml/src/cuda_pipeline/experience_kernels.cu`
+- Modify: `crates/ml/src/cuda_pipeline/gpu_experience_collector.rs`
+
+This is the safety-net layer. Fix the dead code boolean, extend to reversals, update trailing stop threshold, and add reward scaling.
+
+- [ ] **Step 1: Add `min_hold_bars` parameter to `experience_env_step` kernel**
+
+In `experience_kernels.cu`, add `int min_hold_bars` to the kernel signature after the last existing parameter (`raw_returns_out`). Update the `@param` comment block.
+
+- [ ] **Step 2: Pass `min_hold_bars` from Rust**
+
+In `gpu_experience_collector.rs`, add `min_hold_bars: i32` to `ExperienceCollectorConfig` (note: the struct is `ExperienceCollectorConfig`, not `GpuExperienceCollectorConfig`). In the kernel launch (~line 1298-1327), add `.arg(&min_hold_bars_i32)` after the `raw_returns_out` arg. Compute `let min_hold_bars_i32 = config.min_hold_bars as i32;` before the launch. Also compute `let max_pos = config.max_position;` BEFORE the action_select launch (it's currently defined only at line 1293 for env_step — move it up so both launches can use it).
+
+Wire it from `DQNHyperparameters`: in the constructor where config is built, set `min_hold_bars: hyperparams.min_hold_bars as i32`.
+
+- [ ] **Step 3: Replace dead hold guard with unified hold enforcement**
+
+In `experience_kernels.cu`, replace the block at lines 795-814 (the dead early-exit override) with:
+
+```cuda
+    /* ════════════════════════════════════════════════════════════════════
+     * HOLD ENFORCEMENT: block ALL position changes during minimum hold.
+     * Applies to BOTH flat exits AND reversals.
+     *
+     * This replaces the old guard which was dead code:
+     *   `prev_sign != 0 && curr_sign == 0 && !exiting_trade` was always
+     *   false because exiting_trade = (prev_sign != 0 && curr_sign == 0).
+     *
+     * Layer 2 safety net — Layer 1 (action masking) should prevent most
+     * hold violations, but epsilon exploration can still slip through.
+     * ════════════════════════════════════════════════════════════════════ */
+    int wants_exit = (prev_sign != 0 && curr_sign == 0);
+    int wants_reversal = (prev_sign != 0 && curr_sign != 0 && prev_sign != curr_sign);
+    int hold_violation = (hold_time > 0.0f
+                          && hold_time < (float)min_hold_bars
+                          && (wants_exit || wants_reversal)
+                          && bar_idx < total_bars - 1);  /* bypass at episode end */
+
+    if (hold_violation) {
+        /* Override: keep previous position, cancel the action */
+        position = ps[0];
+        cash = ps[1];
+        is_flat = (fabsf(position) < 0.001f) ? 1.0f : 0.0f;
+
+        /* Fix the stored action to match held exposure (prevent action aliasing) */
+        float prev_exposure_frac = ps[0] / (max_position > 0.0f ? max_position : 1.0f);
+        int held_exposure = (int)roundf((prev_exposure_frac + 1.0f) * 0.5f * (float)(b0_size - 1));
+        if (held_exposure < 0) held_exposure = 0;
+        if (held_exposure >= b0_size) held_exposure = b0_size - 1;
+        int original_order = (action_idx / b2_size) % b1_size;
+        int original_urgency = action_idx % b2_size;
+        action_idx = held_exposure * b1_size * b2_size + original_order * b2_size + original_urgency;
+        out_actions[out_off] = action_idx;
+
+        /* Recompute signs from overridden position */
+        curr_sign = prev_sign;
+        wants_exit = 0;
+        wants_reversal = 0;
+    }
+
+    /* Compute final trade lifecycle from (possibly overridden) state */
+    int exiting_trade = wants_exit;
+    int reversing_trade = wants_reversal;
+    int segment_complete = exiting_trade || reversing_trade;
+```
+
+Also REMOVE the old lines 716-720 (`int exiting_trade`, `int reversing_trade`, `int segment_complete`) since they move into the new block. Keep `entering_trade` at line 715 as it's not affected. Add a preliminary `exiting_trade` variable that the trailing stop block at line 790 can write to:
+
+```cuda
+    int entering_trade  = (prev_sign == 0 && curr_sign != 0);
+    int exiting_trade = 0;  /* preliminary — trailing stop may set to 1, hold guard recomputes */
+```
+
+The trailing stop at line 790 sets `exiting_trade = 1;` — this variable must exist before the trailing stop block runs. The hold enforcement block below then recomputes it from the (possibly overridden) position.
+
+- [ ] **Step 4: Update trailing stop threshold**
+
+Change line 780 from:
+```cuda
+    if (prev_sign != 0 && hold_time > 2.0f && peak_equity > 1.0f) {
+```
+to:
+```cuda
+    if (prev_sign != 0 && hold_time >= (float)min_hold_bars && peak_equity > 1.0f) {
+```
+
+The trailing stop only fires after the hold period completes.
+
+- [ ] **Step 5: Add reward scaling for short trades (Layer 3)**
+
+After the existing reward computation (around line 927 where `reward` is fully computed), add:
+
+```cuda
+    /* Layer 3: Reward scaling — shorter trades get proportionally less reward.
+     * Smooth gradient toward longer holds (vs binary cliff at min_hold).
+     * Floor at 10% to maintain gradient signal for very short trades
+     * that slip through (episode boundary, trailing stop override). */
+    if (segment_complete && segment_hold_time > 0.0f && min_hold_bars > 0) {
+        float hold_scale = fminf(segment_hold_time / (float)min_hold_bars, 1.0f);
+        hold_scale = fmaxf(hold_scale, 0.1f);
+        reward *= hold_scale;
+    }
+```
+
+- [ ] **Step 6: Build + test**
+
+Run: `SQLX_OFFLINE=true cargo check -p ml --lib`
+Run: `SQLX_OFFLINE=true cargo test -p ml --lib -- backtest`
+Expected: compiles, backtest tests pass (kernel PTX compiles)
+
+- [ ] **Step 7: Commit**
+
+```bash
+git add crates/ml/src/cuda_pipeline/experience_kernels.cu crates/ml/src/cuda_pipeline/gpu_experience_collector.rs
+git commit -m "fix: hold enforcement in experience_env_step — blocks exits AND reversals during min_hold
+
+Layer 2 (safety net): unified hold guard replaces dead code boolean.
+Old guard (prev_sign!=0 && curr_sign==0 && !exiting_trade) was always
+false. New guard covers both flat exits and reversals.
+
+Layer 3 (gradient): reward scaled by hold_time/min_hold_bars (floor 10%).
+Trailing stop respects min_hold_bars (was hardcoded > 2.0f)."
+```
+
+---
+
+## Task 3: Layer 1 — Action masking in `experience_action_select`
+
+**Files:**
+- Modify: `crates/ml/src/cuda_pipeline/experience_kernels.cu`
+- Modify: `crates/ml/src/cuda_pipeline/gpu_experience_collector.rs`
+
+This is the primary defense layer — prevents the agent from even choosing exit/reversal during the hold period.
+
+- [ ] **Step 1: Add parameters to `experience_action_select` kernel**
+
+In `experience_kernels.cu`, update the `experience_action_select` kernel signature to add portfolio state access:
+
+```cuda
+extern "C" __global__ void experience_action_select(
+    const float*  __restrict__ q_values,
+    int*                       out_actions,
+    unsigned int*              rng_states,
+    float*                     out_q_gaps,
+    float epsilon,
+    int N,
+    int b0_size,
+    int b1_size,
+    int b2_size,
+    float q_gap_threshold,
+    const float* __restrict__ portfolio_states,  /* NEW: [N, PORTFOLIO_STRIDE] */
+    int min_hold_bars,                            /* NEW */
+    float max_position                            /* NEW */
+) {
+```
+
+- [ ] **Step 2: Add action masking logic**
+
+After `int a0, a1, a2;` (line 374), add the hold-time check and masking:
+
+```cuda
+    /* Hold enforcement: during minimum hold period, force current exposure.
+     * Prevents both greedy and random exploration from changing exposure.
+     * Portfolio state: ps[0] = position, ps[10] = hold_time. */
+    int ps_base = i * 20;  /* PORTFOLIO_STRIDE = 20 */
+    float hold_time = portfolio_states[ps_base + 10];
+    float cur_position = portfolio_states[ps_base + 0];
+    int in_hold = (hold_time > 0.0f && hold_time < (float)min_hold_bars
+                   && min_hold_bars > 0);
+
+    if (in_hold) {
+        /* Compute current exposure index from position */
+        float exposure_frac = cur_position / (max_position > 0.0f ? max_position : 1.0f);
+        int cur_exp = (int)roundf((exposure_frac + 1.0f) * 0.5f * (float)(b0_size - 1));
+        if (cur_exp < 0) cur_exp = 0;
+        if (cur_exp >= b0_size) cur_exp = b0_size - 1;
+        a0 = cur_exp;  /* Force current exposure — skip Q-value selection entirely */
+        /* Zero Q-gap during hold — conviction sizing is meaningless when forced */
+        if (out_q_gaps != NULL) {
+            out_q_gaps[i] = 0.0f;
+        }
+    } else {
+```
+
+Then wrap the existing exposure selection (both epsilon and greedy paths) in the `else` branch, and close with `}`.
+
+- [ ] **Step 3: Pass portfolio_states to kernel launch**
+
+In `gpu_experience_collector.rs`, update the action_select kernel launch (~line 1243-1259) to add the three new args:
+
+```rust
+                    .arg(&self.portfolio_states)    // portfolio state for hold masking
+                    .arg(&min_hold_bars_i32)         // min_hold_bars
+                    .arg(&max_pos)                   // max_position (for exposure index)
+```
+
+Add these after `.arg(&q_gap)`.
+
+- [ ] **Step 4: Build + test**
+
+Run: `SQLX_OFFLINE=true cargo check -p ml --lib`
+Run: `SQLX_OFFLINE=true cargo test -p ml --lib -- backtest`
+Expected: compiles, all tests pass
+
+- [ ] **Step 5: Commit**
+
+```bash
+git add crates/ml/src/cuda_pipeline/experience_kernels.cu crates/ml/src/cuda_pipeline/gpu_experience_collector.rs
+git commit -m "feat: action masking in experience_action_select — Layer 1 hold enforcement
+
+During min_hold period, forces exposure to current position index.
+Covers both greedy (Q-value) and random (epsilon) paths.
+Portfolio state already GPU-resident — zero-copy parameter pass."
+```
+
+---
+
+## Task 4: Add `min_hold_bars` to hyperopt search space
+
+**Files:**
+- Modify: `crates/ml/src/hyperopt/adapters/dqn.rs`
+
+- [ ] **Step 1: Add `min_hold_bars` to `DQNParams`**
+
+Find the `DQNParams` struct and add:
+```rust
+    pub min_hold_bars: usize,
+```
+
+In `Default`:
+```rust
+    min_hold_bars: 5,
+```
+
+- [ ] **Step 2: Add to search space bounds**
+
+In `continuous_bounds()`, add a new dimension:
+```rust
+    bounds.push((2.0, 20.0));  // min_hold_bars [2, 20]
+```
+
+In `param_names()`:
+```rust
+    names.push("min_hold_bars");
+```
+
+- [ ] **Step 3: Wire in `from_continuous()` and `to_continuous()`**
+
+In `from_continuous()`:
+```rust
+    params.min_hold_bars = continuous[IDX].round() as usize;
+    params.min_hold_bars = params.min_hold_bars.clamp(2, 20);
+```
+
+In `to_continuous()`:
+```rust
+    vec.push(self.min_hold_bars as f64);
+```
+
+- [ ] **Step 4: Wire to `build_hyperparams()`**
+
+In `build_hyperparams()` where `DQNHyperparameters` is constructed:
+```rust
+    hp.min_hold_bars = params.min_hold_bars;
+```
+
+- [ ] **Step 5: Build + test**
+
+Run: `SQLX_OFFLINE=true cargo check -p ml --lib`
+Run: `SQLX_OFFLINE=true cargo test -p ml --lib -- hyperopt`
+Expected: all hyperopt tests pass (search space dimension increased by 1)
+
+- [ ] **Step 6: Update test assertions for new search space dimension**
+
+Any test that asserts the exact dimension count needs +1. Find with:
+```bash
+grep -rn "continuous_bounds.*len\|param_names.*len\|45\|39" crates/ml/tests/dqn_action_collapse_fix_test.rs
+```
+
+Update `>= 39` to `>= 39` (already flexible from our previous fix) or verify no hardcoded count.
+
+- [ ] **Step 7: Commit**
+
+```bash
+git add crates/ml/src/hyperopt/adapters/dqn.rs
+git commit -m "feat: min_hold_bars in hyperopt search space [2, 20]
+
+Integer parameter (rounded from continuous). Lets PSO discover
+optimal hold duration for ES futures regime characteristics."
+```
+
+---
+
+## Task 5: Action masking in `branching_action_select` (backtest path)
+
+**Files:**
+- Modify: `crates/ml/src/cuda_pipeline/epsilon_greedy_kernel.cu`
+- Modify: `crates/ml/src/cuda_pipeline/gpu_action_selector.rs`
+
+This is the secondary path — used by the backtest evaluator and tests, not the training loop.
+
+- [ ] **Step 1: Add portfolio state parameters to `branching_action_select`**
+
+In `epsilon_greedy_kernel.cu`, add after the `bonus_urgency` parameter:
+
+```cuda
+    const float* __restrict__ portfolio_state,  /* [batch_size, 20] or NULL */
+    int min_hold_bars,
+    float max_position
+```
+
+Note: `portfolio_state` can be NULL when called without hold enforcement (backward compat).
+
+- [ ] **Step 2: Add hold masking logic**
+
+After `int exposure;` (line 192), add:
+
+```cuda
+    /* Hold enforcement (when portfolio_state provided) */
+    int in_hold = 0;
+    int forced_exposure = -1;
+    if (portfolio_state != NULL && min_hold_bars > 0) {
+        float ht = portfolio_state[idx * 20 + 10];  /* hold_time */
+        float pos = portfolio_state[idx * 20 + 0];   /* position */
+        if (ht > 0.0f && ht < (float)min_hold_bars) {
+            in_hold = 1;
+            /* NOTE: branching_action_select currently hardcodes 5 exposure actions.
+             * Use 4.0f = (5-1) for index mapping. If this kernel is later updated
+             * to accept b0_size as a parameter, change to (b0_size-1). */
+            float frac = pos / (max_position > 0.0f ? max_position : 1.0f);
+            forced_exposure = (int)(roundf((frac + 1.0f) * 0.5f * 4.0f));
+            if (forced_exposure < 0) forced_exposure = 0;
+            if (forced_exposure >= 5) forced_exposure = 4;
+        }
+    }
+
+    if (in_hold) {
+        exposure = forced_exposure;
+    } else {
+```
+
+Wrap existing exposure selection in `else { ... }`.
+
+- [ ] **Step 3: Update Rust kernel launch**
+
+In `gpu_action_selector.rs`, pass NULL for portfolio_state and 0 for min_hold_bars (backtest evaluator doesn't enforce holds during evaluation — greedy argmax only):
+
+Add after the existing `.arg(...)` calls:
+```rust
+    .arg(&null_ptr)      // portfolio_state (NULL — no hold enforcement in eval)
+    .arg(&0_i32)         // min_hold_bars (0 = disabled)
+    .arg(&0.0_f32)       // max_position (unused when disabled)
+```
+
+Where `let null_ptr: u64 = 0;` (NULL pointer as device address).
+
+- [ ] **Step 4: Build + test**
+
+Run: `SQLX_OFFLINE=true cargo check -p ml --lib`
+Run: `SQLX_OFFLINE=true cargo test -p ml --lib`
+Expected: 887+ tests pass
+
+- [ ] **Step 5: Commit**
+
+```bash
+git add crates/ml/src/cuda_pipeline/epsilon_greedy_kernel.cu crates/ml/src/cuda_pipeline/gpu_action_selector.rs
+git commit -m "feat: action masking in branching_action_select (backtest/eval path)
+
+Adds portfolio_state + min_hold_bars params (nullable for backward compat).
+Backtest evaluator passes NULL (no hold enforcement during evaluation)."
+```
+
+---
+
+## Task 6: Integration test — validate trade count reduction
+
+**Files:**
+- No new files — validation only
+
+- [ ] **Step 1: Run full ml test suite**
+
+Run: `SQLX_OFFLINE=true cargo test -p ml --lib`
+Expected: 887+ pass, 0 fail
+
+- [ ] **Step 2: Run ml-dqn test suite**
+
+Run: `SQLX_OFFLINE=true cargo test -p ml-dqn --lib`
+Expected: 359 pass, 0 fail
+
+- [ ] **Step 3: Workspace compilation**
+
+Run: `SQLX_OFFLINE=true cargo check --workspace`
+Expected: clean
+
+- [ ] **Step 4: PTX compilation test**
+
+Run: `SQLX_OFFLINE=true cargo test -p ml --lib -- test_metrics_ptx`
+Expected: pass (verifies CUDA kernels compile)
+
+- [ ] **Step 5: Commit all changes and push**
+
+```bash
+git push origin main
+```
+
+---
+
+## Execution Order
+
+```
+Task 1 (config) → Task 2 (Layer 2: kernel override) → Task 3 (Layer 1: action masking)
+    → Task 4 (hyperopt) → Task 5 (backtest path) → Task 6 (integration test)
+```
+
+Tasks 1-3 are sequential (each builds on the previous). Task 4-5 are independent but ordered for clean git history. Task 6 validates everything.
+
+**Expected outcome:** Trade count drops from ~620K to ~120K per epoch (with min_hold_bars=5). The model learns multi-bar swing trading instead of per-bar churning.