Per-component gradient clipping: - 2 new CUDA kernels (dqn_clipped_saxpy, dqn_clip_grad) - CQL gradient isolated into separate scratch buffer - Budget allocation: C51=70%, CQL=15%, IQN=10%, Ens=5% - Dynamic budget — inactive components' share goes to C51 Spectral norm extended to all 10 weight matrices: - Was trunk-only (W_s1, W_s2), now covers all heads - 16 u/v power iteration buffers, batched GOFF sync-back - Uniform sigma_max, end-to-end Lipschitz bounded Bug fixes from 9-agent audit: - Backtest episode reset: all 8 fields (was 5), max_equity updated before floor check - gradient_clip_norm unified: 10.0 everywhere (was 10.0 vs 1.0) - entropy_coefficient: Option<f64> → f64, single default 0.001 - Close price: unwrap_or(1.0) → direct indexing (no silent wrong rewards) - step_count: only increments during training (was incrementing on inference) - Quantile loss: single CUDA context (was 3×), silent fallbacks removed - MaybeNoisyLinear: single-variant enum removed → direct NoisyLinear - Budget fractions: exported as pub(crate) constants, referenced not hardcoded Monitoring: - Per-component gradient Prometheus gauges (C51 raw, combined) - Diagnostic logging every 1000 steps Tests: 7 new gradient budget tests, 1 gradient bounds smoke test All 488 tests pass (359 ml-dqn + 129 ml) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
704 lines
26 KiB
Markdown
704 lines
26 KiB
Markdown
# Design Spec B: DQN CUDA Training Pipeline -- Correctness Bug Fixes
|
|
|
|
**Date**: 2026-03-27
|
|
**Spec**: B of 3 (A = Performance, B = Correctness, C = Dead code)
|
|
**Scope**: 9 CRITICAL + 14 IMPORTANT bugs from 9-agent audit
|
|
**Non-goals**: Performance optimization (Spec A), dead code removal (Spec C)
|
|
|
|
---
|
|
|
|
## 1. Problem Statement
|
|
|
|
A 9-agent audit of the DQN CUDA training pipeline uncovered 23 correctness
|
|
issues. Nine are CRITICAL (produce wrong training results), and 14 are IMPORTANT
|
|
(hide problems via silent fallbacks). One item initially flagged as critical
|
|
(NoisyNet in GPU action selector) was reclassified as NOT A BUG after
|
|
investigation -- see Section 3.9 for the architectural rationale.
|
|
|
|
The bugs fall into four categories:
|
|
|
|
1. **Incomplete state resets** -- backtest kernel resets 5 of 8 portfolio fields
|
|
on episode termination, while the experience kernel resets all 20. Stale
|
|
`max_equity`, `hold_time`, and `entry_price` leak across episodes.
|
|
2. **Inconsistent hyperparameter defaults** -- gradient clip norm defaults to
|
|
10.0 in the main trainer but 1.0 in IQN/IQL sub-trainers; entropy
|
|
coefficient is `Option<f64>` with three different fallback values across
|
|
constructors.
|
|
3. **Silent fallbacks masking data errors** -- `unwrap_or(1.0)` on close prices,
|
|
`unwrap_or(0.0)` on quantile predictions, `unwrap_or(2)` on action indices.
|
|
A single bad data row produces wrong rewards for an entire episode without
|
|
any log message.
|
|
4. **Side effects in read-only paths** -- `step_count` increments during
|
|
inference forward passes, and CUDA contexts are created 3x per quantile
|
|
loss call.
|
|
|
|
---
|
|
|
|
## 2. Files Changed
|
|
|
|
| File | Bugs |
|
|
|------|------|
|
|
| `crates/ml/src/cuda_pipeline/backtest_env_kernel.cu` | #1, #2, #11 |
|
|
| `crates/ml/src/trainers/dqn/data_loading.rs` | #3, #15 |
|
|
| `crates/ml/src/trainers/dqn/fused_training.rs` | #4 |
|
|
| `crates/ml/src/trainers/dqn/trainer/constructor.rs` | #5 |
|
|
| `crates/ml-dqn/src/quantile_regression.rs` | #6, #7 |
|
|
| `crates/ml-dqn/src/network.rs` | #8 |
|
|
| `crates/ml/src/cuda_pipeline/gpu_dqn_trainer.rs` | #10 |
|
|
| `crates/ml/src/cuda_pipeline/experience_kernels.cu` | #14 |
|
|
| `crates/ml-dqn/src/branching.rs` | #16 |
|
|
| `crates/ml/src/trainers/dqn/trainer/action.rs` | #15 |
|
|
| `crates/ml/src/trainers/dqn/trainer/state.rs` | #15 |
|
|
| `crates/ml/src/cuda_pipeline/gpu_backtest_evaluator.rs` | #12, #13 |
|
|
| `crates/ml/src/trainers/dqn/config.rs` | #5 |
|
|
|
|
---
|
|
|
|
## 3. CRITICAL Bugs (9 items -- wrong results)
|
|
|
|
### 3.1 Backtest incomplete episode reset
|
|
|
|
**Location**: `backtest_env_kernel.cu:105-112` (pre-trade floor) and
|
|
`backtest_env_kernel.cu:199-206` (post-trade floor)
|
|
|
|
**Problem**: The backtest kernel uses `PORTFOLIO_STATE_SIZE = 8` fields per
|
|
window. On capital floor breach, both reset blocks only partially reinitialize
|
|
the state:
|
|
|
|
```
|
|
// Current (WRONG) -- pre-trade floor reset (lines 105-112):
|
|
portfolio_state[ps + 0] = liq_value; // value
|
|
portfolio_state[ps + 1] = 0.0f; // position
|
|
portfolio_state[ps + 2] = liq_value; // cash
|
|
portfolio_state[ps + 3] = 0.0f; // entry_price
|
|
portfolio_state[ps + 4] = max_equity; // STALE -- not updated to new_value
|
|
portfolio_state[ps + 5] = 0.0f; // hold_time
|
|
portfolio_state[ps + 6] = cum_return + liq_ret; // ACCUMULATES across episodes
|
|
portfolio_state[ps + 7] += 1.0f; // step_count INCREMENTS, never resets
|
|
```
|
|
|
|
Compare with the experience kernel (`experience_kernels.cu:614-630`) which
|
|
resets all 20 fields to fresh-episode values: position=0, cash=initial_capital,
|
|
portfolio_value=initial_capital, peak_equity=initial_capital, and zeroes all
|
|
counters.
|
|
|
|
**Impact**: After a capital floor breach, the backtest continues the next
|
|
episode with (a) stale `max_equity` from the blown episode, which prevents the
|
|
floor check from firing correctly in subsequent episodes, (b) accumulated
|
|
`cum_return` that conflates multiple episodes, and (c) a `step_count` that
|
|
never resets, making per-episode metrics meaningless.
|
|
|
|
**Fix**: Reset all 8 backtest portfolio fields to initial-episode state at both
|
|
reset sites. Use `liq_value` (or `new_value` for the post-trade block) as the
|
|
new initial capital:
|
|
|
|
```
|
|
// Pre-trade floor reset (lines 105-112):
|
|
portfolio_state[ps + 0] = liq_value; // value = initial capital for next ep
|
|
portfolio_state[ps + 1] = 0.0f; // position = flat
|
|
portfolio_state[ps + 2] = liq_value; // cash = initial capital
|
|
portfolio_state[ps + 3] = 0.0f; // entry_price = none
|
|
portfolio_state[ps + 4] = liq_value; // max_equity = reset to new initial
|
|
portfolio_state[ps + 5] = 0.0f; // hold_time = 0
|
|
portfolio_state[ps + 6] = 0.0f; // cum_return = reset for new episode
|
|
portfolio_state[ps + 7] = 0.0f; // step_count = reset for new episode
|
|
|
|
// Post-trade floor reset (lines 199-206): identical pattern with new_value
|
|
portfolio_state[ps + 0] = new_value;
|
|
portfolio_state[ps + 1] = 0.0f;
|
|
portfolio_state[ps + 2] = new_value;
|
|
portfolio_state[ps + 3] = 0.0f;
|
|
portfolio_state[ps + 4] = new_value; // max_equity = reset
|
|
portfolio_state[ps + 5] = 0.0f;
|
|
portfolio_state[ps + 6] = 0.0f; // cum_return = reset
|
|
portfolio_state[ps + 7] = 0.0f; // step_count = reset
|
|
```
|
|
|
|
**Testing**: Add a backtest scenario where a window triggers the capital floor.
|
|
Verify that the next episode starts with fresh `max_equity` equal to the
|
|
liquidated equity, `cum_return = 0`, and `step_count = 0`.
|
|
|
|
---
|
|
|
|
### 3.2 Backtest stale max_equity before post-trade floor check
|
|
|
|
**Location**: `backtest_env_kernel.cu:184`
|
|
|
|
**Problem**: The post-trade capital floor check at line 184 runs
|
|
`check_capital_floor(new_value, max_equity)` using the *pre-trade* `max_equity`.
|
|
If the trade was profitable enough to set a new high-water mark, the floor
|
|
check uses a stale denominator, potentially missing a breach or triggering a
|
|
false one.
|
|
|
|
The normal (non-done) path updates `max_equity` at line 214:
|
|
`float new_max = fmaxf(max_equity, new_value);` -- but this happens *after*
|
|
the floor check.
|
|
|
|
**Fix**: Insert the max_equity update before the floor check:
|
|
|
|
```
|
|
float new_value = cash + position * close;
|
|
|
|
// Update max_equity BEFORE floor check
|
|
max_equity = fmaxf(max_equity, new_value);
|
|
|
|
if (check_capital_floor(new_value, max_equity)) {
|
|
...
|
|
```
|
|
|
|
**Testing**: Construct a scenario where a profitable trade raises equity above
|
|
the previous peak, then verify the floor check uses the updated peak.
|
|
|
|
---
|
|
|
|
### 3.3 Close price defaults to 1.0
|
|
|
|
**Location**: `data_loading.rs:306`
|
|
|
|
**Problem**: When computing simple returns, the code uses:
|
|
```rust
|
|
let prev = close_prices_f32.get(i - 1).copied().unwrap_or(1.0);
|
|
let curr = close_prices_f32.get(i).copied().unwrap_or(prev);
|
|
```
|
|
|
|
If `close_prices_f32` contains any parse failures that result in a missing
|
|
value, the fallback `1.0` is a valid-looking but completely wrong price
|
|
(ES trades around 5000). The resulting return `(5000 - 1) / 1 = 4999` or
|
|
`(1 - 5000) / 5000 = -0.9998` will dominate the entire episode's reward
|
|
signal.
|
|
|
|
Since `close_prices_f32` is an indexed `Vec<f32>` accessed by in-bounds index
|
|
`i-1` and `i` where `i in 1..n`, the `.get()` calls can never actually return
|
|
`None` -- the indices are always valid. However, the `unwrap_or(1.0)` gives
|
|
a false sense of safety and suppresses any future index bugs.
|
|
|
|
**Fix**: Replace `unwrap_or` with direct indexing (which panics on OOB,
|
|
correctly surfacing the bug):
|
|
|
|
```rust
|
|
let prev = close_prices_f32[i - 1];
|
|
let curr = close_prices_f32[i];
|
|
```
|
|
|
|
Alternatively, use `expect("close price index in bounds")` if a message is
|
|
preferred over a bare panic.
|
|
|
|
**Testing**: Existing tests cover the happy path. Add a `#[should_panic]` test
|
|
that calls the return computation with an empty `close_prices_f32` to confirm
|
|
the panic behavior.
|
|
|
|
---
|
|
|
|
### 3.4 gradient_clip_norm inconsistent defaults
|
|
|
|
**Location**: `fused_training.rs:194,263,298`
|
|
|
|
**Problem**: Three config construction sites read `gradient_clip_norm` from the
|
|
same `Option<f64>` hyperparameter but use different defaults:
|
|
|
|
| Site | Default | Component |
|
|
|------|---------|-----------|
|
|
| `fused_training.rs:194` | `unwrap_or(10.0)` | Main C51 trainer |
|
|
| `fused_training.rs:263` | `unwrap_or(1.0)` | IQL value trainer |
|
|
| `fused_training.rs:298` | `unwrap_or(1.0)` | IQN dual-head trainer |
|
|
| `constructor.rs:257` | `unwrap_or(10.0)` | CPU-side DQN config |
|
|
|
|
When hyperopt omits `gradient_clip_norm`, the main trainer clips at 10.0 while
|
|
IQN/IQL clip at 1.0. The gradient budget fractions in `gpu_dqn_trainer.rs`
|
|
(line 857: `max_grad_norm * 0.10` for IQN, line 1098: `max_grad_norm * 0.05`
|
|
for ensemble) assume the main norm is the ceiling. If the sub-trainers
|
|
independently clip to 1.0, the total gradient budget accounting is wrong.
|
|
|
|
**Fix**: Make `gradient_clip_norm` required (non-optional) in `DqnHyperparams`.
|
|
Set the canonical default in `DqnHyperparams::default()` to `10.0`. Remove all
|
|
`unwrap_or()` fallbacks -- the value is always present:
|
|
|
|
```rust
|
|
// In DqnHyperparams:
|
|
pub gradient_clip_norm: f64, // was Option<f64>
|
|
|
|
// All construction sites:
|
|
max_grad_norm: hyperparams.gradient_clip_norm as f32,
|
|
```
|
|
|
|
If making the field non-optional is too disruptive for serialization
|
|
compatibility, keep `Option<f64>` but resolve it once at the top of
|
|
`FusedTrainingCtx::new()` and pass the resolved value to all sub-configs:
|
|
|
|
```rust
|
|
let grad_norm = hyperparams.gradient_clip_norm.unwrap_or(10.0);
|
|
// Use `grad_norm` everywhere -- single source of truth
|
|
```
|
|
|
|
**Testing**: Verify that a training run with `gradient_clip_norm = None`
|
|
produces the same effective clip norm (10.0) in all three sub-trainers. Add an
|
|
assertion in `FusedTrainingCtx::new()` that the IQN and IQL configs use the
|
|
same `max_grad_norm` as the main config.
|
|
|
|
---
|
|
|
|
### 3.5 entropy_coefficient Option mismatch
|
|
|
|
**Location**: `config.rs:960`, `constructor.rs:314,399`, `dqn.rs:287,740`,
|
|
`hyperopt/adapters/dqn.rs:418,2553`
|
|
|
|
**Problem**: The `entropy_coefficient` field has a fragmented type/default chain:
|
|
|
|
| Location | Type | Default |
|
|
|----------|------|---------|
|
|
| `DqnHyperparams` (`config.rs:960`) | `Option<f64>` | `None` |
|
|
| `DQNConfig` (`dqn.rs:287`) | `f64` (non-optional) | `0.001` |
|
|
| `constructor.rs:314` | unwraps Option | `unwrap_or(0.01)` |
|
|
| `constructor.rs:399` | unwraps Option | `unwrap_or(0.01)` |
|
|
| Hyperopt default (`adapters/dqn.rs:418`) | `f64` | `0.02` |
|
|
| Hyperopt search range (`adapters/dqn.rs:517`) | `f64` | `(0.05, 0.5)` |
|
|
| Hyperopt to hyperparams (`adapters/dqn.rs:2553`) | `Some(f64)` | from PSO |
|
|
|
|
The constructor's `unwrap_or(0.01)` disagrees with `DQNConfig::default()`
|
|
(0.001) and with hyperopt's default (0.02). When hyperopt runs, it always
|
|
provides a value via `Some(params.entropy_coefficient)`, so the disagreement
|
|
only matters for manual (non-hyperopt) training runs.
|
|
|
|
**Fix**: Make `entropy_coefficient` non-optional in `DqnHyperparams`. Set the
|
|
canonical default to match the hyperopt search space center. Remove the
|
|
`unwrap_or` in the constructor:
|
|
|
|
```rust
|
|
// In DqnHyperparams (config.rs):
|
|
pub entropy_coefficient: f64, // was Option<f64>
|
|
|
|
// In constructor.rs:
|
|
entropy_coefficient: hyperparams.entropy_coefficient,
|
|
```
|
|
|
|
Update `DqnHyperparams::default()` to use `0.01` as the default (midpoint
|
|
between DQNConfig's 0.001 and hyperopt's 0.02 on log scale, and matching the
|
|
old `unwrap_or`).
|
|
|
|
**Testing**: Grep for all `entropy_coefficient.*unwrap` after the change and
|
|
confirm zero hits in production code.
|
|
|
|
---
|
|
|
|
### 3.6 3x CUDA context creation in quantile loss
|
|
|
|
**Location**: `quantile_regression.rs:320-346`
|
|
|
|
**Problem**: `quantile_huber_loss()` creates a new `CudaContext::new(0)` three
|
|
separate times in a single call:
|
|
|
|
1. Line 320-326: for the empty-batch early return `GpuTensor::scalar(0.0)`
|
|
2. Line 332-338: for `per_sample.to_host()` stream
|
|
3. Line 341-346: for the final `GpuTensor::scalar(mean_loss)`
|
|
|
|
Each `CudaContext::new(0)` call invokes `cuCtxCreate` or
|
|
`cuDevicePrimaryCtxRetain`, which is a heavyweight driver call. On the hot
|
|
training path, this is called once per batch.
|
|
|
|
Additionally, `quantile_huber_loss_per_sample()` creates its own context at
|
|
line 369-374, for a total of 4 context creations per quantile loss computation.
|
|
|
|
**Fix**: Create the context once at the top of `quantile_huber_loss()` and pass
|
|
it (or the stream derived from it) to all operations:
|
|
|
|
```rust
|
|
pub fn quantile_huber_loss(...) -> Result<GpuTensor, MLError> {
|
|
let ctx = cudarc::driver::CudaContext::new(0)
|
|
.map_err(|e| MLError::DeviceError(format!("CUDA context: {e}")))?;
|
|
let stream = ctx.new_stream()
|
|
.map_err(|e| MLError::DeviceError(format!("CUDA stream: {e}")))?;
|
|
|
|
let per_sample = quantile_huber_loss_per_sample_with_stream(
|
|
predicted, target, taus, kappa, &stream
|
|
)?;
|
|
|
|
let batch = per_sample.numel();
|
|
if batch == 0 {
|
|
return GpuTensor::scalar(0.0, &stream);
|
|
}
|
|
|
|
let host = per_sample.to_host(&stream)?;
|
|
let mean_loss: f32 = host.iter().sum::<f32>() / batch as f32;
|
|
GpuTensor::scalar(mean_loss, &stream)
|
|
}
|
|
```
|
|
|
|
Extract the inner loop of `quantile_huber_loss_per_sample` into a
|
|
`_with_stream` variant that accepts an existing `&CudaStream`.
|
|
|
|
**Testing**: Existing quantile loss tests cover correctness. No new tests
|
|
needed -- this is a pure refactor. Verify tests pass after the change.
|
|
|
|
---
|
|
|
|
### 3.7 Silent 0.0/0.5 fallback in quantile loss
|
|
|
|
**Location**: `quantile_regression.rs:385-387`
|
|
|
|
**Problem**: Inside the per-sample quantile Huber loss loop:
|
|
|
|
```rust
|
|
let pred_val = pred_host.get(idx).copied().unwrap_or(0.0);
|
|
let tgt_val = tgt_host.get(idx).copied().unwrap_or(0.0);
|
|
let tau_val = tau_host.get(idx).copied().unwrap_or(0.5);
|
|
```
|
|
|
|
The index `idx = b * num_q + q` with `b in 0..batch` and `q in 0..num_q`
|
|
should always be in bounds since `pred_host` has exactly `batch * num_q`
|
|
elements. If it is ever out of bounds, the loss computation silently uses
|
|
`pred=0.0`, `target=0.0` (producing zero loss -- masking real errors) or
|
|
`tau=0.5` (symmetric quantile -- incorrect for risk-sensitive IQN).
|
|
|
|
**Fix**: Replace `unwrap_or` with direct indexing or `expect`:
|
|
|
|
```rust
|
|
let pred_val = pred_host[idx];
|
|
let tgt_val = tgt_host[idx];
|
|
let tau_val = tau_host[idx];
|
|
```
|
|
|
|
If bounds checking is desired for safety, use:
|
|
|
|
```rust
|
|
let pred_val = *pred_host.get(idx).ok_or_else(|| {
|
|
MLError::ModelError(format!(
|
|
"pred OOB: idx={idx}, len={}", pred_host.len()
|
|
))
|
|
})?;
|
|
```
|
|
|
|
**Testing**: Existing tests. Add a test with mismatched tensor shapes to verify
|
|
the error is returned (not silently swallowed).
|
|
|
|
---
|
|
|
|
### 3.8 step_count increments during inference
|
|
|
|
**Location**: `network.rs:250`
|
|
|
|
**Problem**: The `forward()` method on the DQN network unconditionally
|
|
increments `step_count`:
|
|
|
|
```rust
|
|
let _step = self.step_count.fetch_add(1, Ordering::Relaxed);
|
|
```
|
|
|
|
This counter is used by the dropout scheduler (line 253-257) and potentially
|
|
by other scheduling logic. During inference (backtest, action selection during
|
|
epsilon-greedy exploration), the forward pass is read-only -- it should not
|
|
advance the training schedule.
|
|
|
|
Every inference forward pass advances the dropout schedule by one step, causing
|
|
dropout to decay faster than intended.
|
|
|
|
**Fix**: Add a `training: bool` parameter to `forward()`, or split into
|
|
`forward_train()` and `forward_eval()`:
|
|
|
|
Option A -- parameter:
|
|
```rust
|
|
pub fn forward(&self, state: &[f32], training: bool) -> Result<Vec<f32>, MLError> {
|
|
// ... existing logic ...
|
|
if training {
|
|
let _step = self.step_count.fetch_add(1, Ordering::Relaxed);
|
|
if let Ok(mut scheduler_opt) = self.dropout_scheduler.lock() {
|
|
if let Some(scheduler) = scheduler_opt.as_mut() {
|
|
scheduler.step(1);
|
|
}
|
|
}
|
|
}
|
|
Ok(output_vec)
|
|
}
|
|
```
|
|
|
|
Option B -- separate methods (preferred for API clarity):
|
|
```rust
|
|
pub fn forward_impl(&self, state: &[f32], training: bool) -> Result<Vec<f32>, MLError> {
|
|
// ... shared logic with conditional step_count ...
|
|
}
|
|
|
|
pub fn forward_train(&self, state: &[f32]) -> Result<Vec<f32>, MLError> {
|
|
self.forward_impl(state, true)
|
|
}
|
|
|
|
pub fn forward_for_eval(&self, state: &[f32]) -> Result<Vec<f32>, MLError> {
|
|
self.forward_impl(state, false)
|
|
}
|
|
```
|
|
|
|
All inference call sites (backtest, action selection in greedy mode) must call
|
|
the non-training variant. All training call sites must call the train variant.
|
|
|
|
**Testing**: Call the non-training forward 1000 times and verify `step_count`
|
|
does not change. Call the training forward once and verify it increments by 1.
|
|
|
|
---
|
|
|
|
### 3.9 NoisyNet noise in GPU action selector -- NOT A BUG
|
|
|
|
**Location**: `gpu_action_selector.rs`
|
|
|
|
**Initial finding**: The GPU action selector uses pure epsilon-greedy; there is
|
|
no explicit Gaussian noise injection for NoisyNet exploration.
|
|
|
|
**Analysis**: NoisyNet (Fortunato et al., 2018) adds parametric noise to
|
|
linear layer weights. In this codebase, `NoisyLinear` layers are part of the
|
|
`BranchingDuelingNetwork`. The noise is injected into the weight matrices
|
|
during the forward pass, which means the Q-values output by the network
|
|
*already incorporate NoisyNet exploration*. The action selector receives
|
|
noise-perturbed Q-values and simply takes the argmax (or applies
|
|
epsilon-greedy as a fallback).
|
|
|
|
This is the standard NoisyNet architecture. The selector does not need
|
|
separate noise injection -- that would double-count the exploration.
|
|
|
|
**Verdict**: Not a bug. Document this architectural choice with a comment in
|
|
`gpu_action_selector.rs`:
|
|
|
|
```rust
|
|
// NoisyNet exploration note: Q-values received here already include
|
|
// factorized Gaussian noise from NoisyLinear layers in the forward pass.
|
|
// Epsilon-greedy acts as a secondary fallback, not the primary exploration
|
|
// mechanism when NoisyNet is enabled.
|
|
```
|
|
|
|
---
|
|
|
|
## 4. IMPORTANT Bugs (14 items -- hiding problems)
|
|
|
|
### 4.10 Budget fractions hardcoded in gpu_dqn_trainer
|
|
|
|
**Location**: `gpu_dqn_trainer.rs:857,1098`
|
|
|
|
**Problem**:
|
|
```rust
|
|
let max_component_norm = self.config.max_grad_norm * 0.10; // IQN_GRAD_BUDGET
|
|
let max_component_norm = self.config.max_grad_norm * 0.05; // ENS_GRAD_BUDGET
|
|
```
|
|
|
|
These magic numbers define what fraction of the total gradient budget is
|
|
allocated to auxiliary components (IQN, ensemble). They should be configurable
|
|
or at least centralized as named constants.
|
|
|
|
**Fix**: Define constants in `fused_training.rs`:
|
|
|
|
```rust
|
|
/// Fraction of max_grad_norm allocated to IQN gradient updates.
|
|
pub const IQN_GRAD_BUDGET_FRACTION: f32 = 0.10;
|
|
|
|
/// Fraction of max_grad_norm allocated to ensemble gradient updates.
|
|
pub const ENSEMBLE_GRAD_BUDGET_FRACTION: f32 = 0.05;
|
|
```
|
|
|
|
Reference these from `gpu_dqn_trainer.rs`. Alternatively, add them to the
|
|
`FusedConfig` struct if hyperopt should search over them.
|
|
|
|
---
|
|
|
|
### 4.11 Trailing stop distance hardcoded
|
|
|
|
**Location**: `backtest_env_kernel.cu:151`
|
|
|
|
**Problem**: `check_trailing_stop(..., 0.005f, ...)` -- the 0.5% base distance
|
|
is ES-specific and not configurable via kernel arguments.
|
|
|
|
**Fix**: Add `trailing_stop_base_dist` to the kernel parameter list and pass
|
|
it from `GpuBacktestConfig`. Default to `0.005` for backward compatibility.
|
|
|
|
---
|
|
|
|
### 4.12-4.13 ES-specific defaults in GpuBacktestConfig
|
|
|
|
**Location**: `gpu_backtest_evaluator.rs:205,211`
|
|
|
|
**Problem**: `contract_multiplier: 50.0` and `margin_pct: 0.06` are ES-specific
|
|
defaults. These are already configurable fields in `GpuBacktestConfig`, so the
|
|
fix is documentation, not code.
|
|
|
|
**Fix**: Add doc comments to both fields in the `Default` impl:
|
|
|
|
```rust
|
|
/// ES mini S&P 500 default. Override for other instruments:
|
|
/// NQ=20.0, CL=1000.0, GC=100.0, etc.
|
|
contract_multiplier: 50.0,
|
|
/// ES initial margin percentage (CME). Override for other instruments.
|
|
margin_pct: 0.06,
|
|
```
|
|
|
|
---
|
|
|
|
### 4.14 Kelly min trades hardcoded
|
|
|
|
**Location**: `experience_kernels.cu:670`
|
|
|
|
**Problem**: `if (total_trades >= 20.0f)` -- the minimum number of trades
|
|
before Kelly criterion activates is hardcoded. For different bar frequencies
|
|
(daily vs 1-min), 20 trades may be too few or too many.
|
|
|
|
**Fix**: Add `kelly_min_trades` as a kernel parameter. Default to `20` for
|
|
backward compatibility. Pass from `DqnBacktestConfig` or derive from
|
|
`bars_per_day`.
|
|
|
|
---
|
|
|
|
### 4.15 Silent unwrap_or fallbacks in production paths
|
|
|
|
**Priority tiers**:
|
|
|
|
**Tier 1 -- data correctness (fix now)**:
|
|
- `data_loading.rs:306` -- `unwrap_or(1.0)` on close price (covered in #3)
|
|
- `action.rs:161` -- `unwrap_or(2)` on argmax result (flat action fallback).
|
|
The argmax should never fail on a non-empty tensor. Replace with
|
|
`.expect("argmax on non-empty Q-values")`.
|
|
- `state.rs:73` -- `unwrap_or(0.0)` on price `to_f32()`. A zero price
|
|
produces zero returns. Replace with `.expect("price fits f32")` since
|
|
trading prices always fit in f32.
|
|
|
|
**Tier 2 -- metrics/monitoring (fix later, lower risk)**:
|
|
- `training_stability.rs` (15 instances) -- test code using `unwrap_or(f64::NAN)`
|
|
on metric lookups. These are acceptable since NaN propagates to test failures.
|
|
- `metrics.rs` (10 instances) -- monitoring aggregation using `unwrap_or(0.0)`.
|
|
Missing metrics produce zeros in dashboards, which is misleading but not
|
|
training-breaking. Replace with `unwrap_or(f64::NAN)` where appropriate.
|
|
- `config.rs:300,419,773,774,788,789` -- shape/dimension lookups with fallbacks
|
|
to 0 or 256. These are initialization-time and generally safe.
|
|
|
|
**Tier 3 -- test code (leave as-is)**:
|
|
- `smoke_tests/helpers.rs`, `tests.rs` -- test utilities. Fallbacks are
|
|
acceptable in test code.
|
|
|
|
---
|
|
|
|
### 4.16 MaybeNoisyLinear single-variant enum
|
|
|
|
**Location**: `branching.rs:252-324`
|
|
|
|
**Problem**: The `MaybeNoisyLinear` enum has exactly one variant:
|
|
|
|
```rust
|
|
enum MaybeNoisyLinear {
|
|
Noisy(NoisyLinear),
|
|
}
|
|
```
|
|
|
|
A `Linear` variant was presumably removed when the architecture converged on
|
|
always-noisy layers. The enum wrapper adds indirection, match arms, and
|
|
cognitive overhead for no benefit.
|
|
|
|
**Fix**: Replace all occurrences of `MaybeNoisyLinear` with `NoisyLinear`
|
|
directly. The methods on `MaybeNoisyLinear` (`forward`, `reset_noise`,
|
|
`reset_noise_with_sigma`, `disable_noise`, `register_mu_in_varstore`,
|
|
`noisy_sigma_slices`) are thin wrappers -- inline them.
|
|
|
|
Fields in `BranchingDuelingNetwork` change from:
|
|
```rust
|
|
value_fc: MaybeNoisyLinear,
|
|
value_out: MaybeNoisyLinear,
|
|
branch_fcs: Vec<MaybeNoisyLinear>,
|
|
branch_outs: Vec<MaybeNoisyLinear>,
|
|
```
|
|
to:
|
|
```rust
|
|
value_fc: NoisyLinear,
|
|
value_out: NoisyLinear,
|
|
branch_fcs: Vec<NoisyLinear>,
|
|
branch_outs: Vec<NoisyLinear>,
|
|
```
|
|
|
|
**Testing**: Compile-only change. All existing tests cover the behavior.
|
|
|
|
---
|
|
|
|
### 4.17-4.20 Lower-priority silent fallbacks
|
|
|
|
These items are acknowledged but deprioritized:
|
|
|
|
- **4.17**: `action.rs:96-101` -- count bonus `unwrap_or(0.0)` on branch
|
|
exploration arrays. Safe when `count_bonus_coefficient = 0.0` (disabled).
|
|
Fix: bounds-check the slice access.
|
|
- **4.18**: `attention.rs` fallbacks -- attention mechanism defaults.
|
|
Low risk since attention is not on the critical training path.
|
|
- **4.19**: PER beta defaults -- `unwrap_or` on PER importance sampling beta.
|
|
Fix: make PER beta required in config (it always should be present when PER
|
|
is enabled).
|
|
- **4.20**: Hyperopt validation gaps -- PSO can produce out-of-range values
|
|
that get clamped silently. Fix: add `validate()` method to
|
|
`DqnPsoParams` that returns `Result` instead of clamping.
|
|
|
|
---
|
|
|
|
## 5. Implementation Plan
|
|
|
|
### Phase 1: CRITICAL fixes (bugs #1-#8)
|
|
|
|
All changes in this phase affect training correctness and must be deployed
|
|
together.
|
|
|
|
| Order | Bug | Risk | Reason for ordering |
|
|
|-------|-----|------|---------------------|
|
|
| 1 | #4 gradient_clip_norm | Low | Config-only, no logic change |
|
|
| 2 | #5 entropy_coefficient | Low | Config-only, no logic change |
|
|
| 3 | #3 close price fallback | Low | Replace unwrap_or with indexing |
|
|
| 4 | #8 step_count inference | Medium | API change to forward() |
|
|
| 5 | #6 CUDA context 3x | Low | Pure refactor |
|
|
| 6 | #7 quantile fallbacks | Low | Replace unwrap_or with error |
|
|
| 7 | #1 backtest episode reset | Medium | CUDA kernel change |
|
|
| 8 | #2 stale max_equity | Medium | CUDA kernel change, order matters |
|
|
|
|
### Phase 2: IMPORTANT fixes (bugs #10-#16)
|
|
|
|
These can be deployed incrementally.
|
|
|
|
| Order | Bug | Risk |
|
|
|-------|-----|------|
|
|
| 1 | #16 MaybeNoisyLinear | Low (compile-only) |
|
|
| 2 | #10 budget fraction constants | Low |
|
|
| 3 | #15 Tier 1 unwrap_or fixes | Low |
|
|
| 4 | #11 trailing stop param | Medium (kernel API change) |
|
|
| 5 | #14 Kelly min trades param | Medium (kernel API change) |
|
|
| 6 | #12-#13 doc comments | None |
|
|
|
|
### Phase 3: Lower priority (#17-#20)
|
|
|
|
Deferred to a follow-up PR.
|
|
|
|
---
|
|
|
|
## 6. Success Criteria
|
|
|
|
1. **Zero `unwrap_or` on data-critical paths**: `grep -rn 'unwrap_or'` in
|
|
`data_loading.rs`, `quantile_regression.rs`, `action.rs:161`, and
|
|
`state.rs:73` returns zero hits on the specific lines identified.
|
|
|
|
2. **Backtest episode reset matches experience kernel**: After capital floor
|
|
breach, all 8 backtest portfolio fields are reset to fresh-episode values.
|
|
`max_equity` is set to the new initial capital, not the stale peak.
|
|
|
|
3. **Gradient clip norm consistency**: A single `gradient_clip_norm` value
|
|
propagates to all sub-trainers. Searching for `unwrap_or.*gradient_clip`
|
|
returns zero hits.
|
|
|
|
4. **entropy_coefficient consistency**: The field is non-optional (or resolved
|
|
once). No `unwrap_or` on entropy_coefficient in production code.
|
|
|
|
5. **No inference side effects**: The non-training forward pass does not
|
|
increment `step_count` or advance the dropout scheduler.
|
|
|
|
6. **All 129+ existing tests pass**: `SQLX_OFFLINE=true cargo test -p ml --lib`
|
|
and `SQLX_OFFLINE=true cargo test -p ml-dqn --lib` both pass.
|
|
|
|
7. **Smoke test passes**: `FOXHUNT_TEST_DATA=test_data/futures-baseline cargo
|
|
test -p ml --lib -- smoke_tests --ignored --nocapture` completes without
|
|
regression.
|
|
|
|
---
|
|
|
|
## 7. Risks and Mitigations
|
|
|
|
| Risk | Mitigation |
|
|
|------|------------|
|
|
| Making `gradient_clip_norm` non-optional breaks deserialization of saved hyperparams | Use `#[serde(default = "default_grad_clip")]` with `fn default_grad_clip() -> f64 { 10.0 }` |
|
|
| Making `entropy_coefficient` non-optional breaks deserialization | Same `#[serde(default)]` strategy |
|
|
| Changing `forward()` signature breaks call sites | Use `forward_train()` / non-training forward split; deprecate `forward()` |
|
|
| Backtest reset changes affect hyperopt metric consistency | Run a before/after comparison on the smoke test dataset to quantify metric drift |
|
|
| Kernel parameter additions (#11, #14) require host-side changes | Add the new parameters to `GpuBacktestConfig` with defaults matching current hardcoded values -- zero behavioral change by default |
|