From 660b3f5b509cbffed61da501ac1b23b68d54c182 Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Sun, 22 Mar 2026 21:27:00 +0100 Subject: [PATCH] =?UTF-8?q?refactor:=20remove=20dead=20CPU=20reward=20code?= =?UTF-8?q?=20=E2=80=94=20RewardNormalizer,=20use=5Fdsr=20toggle,=20hold?= =?UTF-8?q?=5Freward?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reward normalization and DSR computation have been moved to the GPU experience-collection kernel (experience_kernels.cu). This removes the now-dead CPU-side code: - Remove RewardNormalizer struct (150 lines) — GPU pnl_ema/pnl_var replaces it - Remove hold_reward, hold_penalty_weight, enable_normalization from RewardConfig - Remove use_dsr toggle from RewardConfig and RewardConfigBuilder — DSR is always on - Remove calculate_hold_reward() — inlined as Decimal::ZERO (GPU w_idle replaces it) - Make RewardFunction.dsr non-optional (always enabled) - Make training_loop.rs DSR sync + epoch reset unconditional - Clean up constructor.rs RewardConfig construction (6 fewer fields) - Mark DQNHyperparameters.use_dsr and hold_penalty_weight as deprecated Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/ml-dqn/src/reward.rs | 360 ++---------------- crates/ml/src/trainers/dqn/config.rs | 7 +- .../src/trainers/dqn/trainer/constructor.rs | 8 +- .../src/trainers/dqn/trainer/training_loop.rs | 33 +- 4 files changed, 48 insertions(+), 360 deletions(-) diff --git a/crates/ml-dqn/src/reward.rs b/crates/ml-dqn/src/reward.rs index 740b92216..72f3c7e5b 100644 --- a/crates/ml-dqn/src/reward.rs +++ b/crates/ml-dqn/src/reward.rs @@ -12,159 +12,6 @@ use super::agent::{TradingAction, TradingState}; use ml_core::MLError; use super::circuit_breaker::CircuitBreakerConfig; -/// Online reward normalization using Exponential Moving Average (EMA) -/// -/// Implements incremental mean and variance calculation to normalize rewards -/// to a standard normal distribution ~N(0,1). Uses EMA to prevent mean drift -/// in non-stationary markets (BUG #41 fix). -/// -/// # Algorithm: Exponential Moving Average (EMA) -/// - Adapts to non-stationary reward distributions -/// - O(1) memory (no need to store all values) -/// - Exponentially decaying weights for old samples -/// -/// # Why EMA over Welford's? -/// Welford's algorithm gives equal weight to all historical samples, causing -/// mean drift in non-stationary markets. When the market produces losses, -/// the mean drifts negative and stays there, inverting reward signs. -/// EMA gives exponentially decaying weight to old samples, adapting to regime changes. -/// -/// # Usage -/// ```ignore -/// let mut normalizer = RewardNormalizer::new(); -/// for reward in rewards { -/// normalizer.update(reward); -/// let normalized = normalizer.normalize(reward); -/// } -/// ``` -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct RewardNormalizer { - /// Running mean - mean: f64, - /// Running variance - variance: f64, - /// Mean decay rate (default: 0.01 = ~100-step effective window) - alpha: f64, - /// Variance decay rate (default: 0.01 = ~100-step effective window) - beta: f64, - /// Whether the normalizer has been initialized with at least one sample - initialized: bool, - /// Small constant for numerical stability - epsilon: f64, -} - -impl RewardNormalizer { - /// Create a new reward normalizer with default decay rates - /// - /// # Default Parameters - /// - alpha (mean decay): 0.01 = ~100-step effective window - /// - beta (variance decay): 0.01 = ~100-step effective window - /// - /// # Effective Window Calculation - /// For decay rate α, the effective window is approximately 1/α samples. - /// Examples: - /// - α=0.01 → 100 steps (balanced: reactive yet stable) - /// - α=0.05 → 20 steps (aggressive: very reactive to changes) - /// - α=0.001 → 1000 steps (conservative: smooth but slow) - pub const fn new() -> Self { - Self { - mean: 0.0, - variance: 1.0, - alpha: 0.01, - beta: 0.01, - initialized: false, - epsilon: 1e-8, - } - } - - /// Update running statistics with a new value (EMA algorithm) - /// - /// # Algorithm - /// ```text - /// First sample: - /// mean = value - /// variance = 1.0 - /// initialized = true - /// - /// Subsequent samples: - /// mean = alpha * value + (1 - alpha) * mean - /// diff = value - mean - /// variance = beta * diff^2 + (1 - beta) * variance - /// ``` - /// - /// # Why This Prevents Mean Drift - /// - Old samples decay exponentially: weight = (1-α)^t - /// - After 100 steps (α=0.01): weight = 0.366 (63.4% decay) - /// - After 200 steps: weight = 0.134 (86.6% decay) - /// - After 500 steps: weight = 0.007 (99.3% decay) - /// - /// This allows the mean to "forget" old regimes and adapt to new ones. - pub fn update(&mut self, value: f64) { - if !self.initialized { - // First sample: initialize mean and variance - self.mean = value; - self.variance = 1.0; - self.initialized = true; - } else { - // EMA update for mean - self.mean = self.alpha * value + (1.0 - self.alpha) * self.mean; - - // EMA update for variance - let diff = value - self.mean; - self.variance = self.beta * diff.powi(2) + (1.0 - self.beta) * self.variance; - } - } - - /// Normalize a value to standard normal distribution - /// - /// # Returns - /// `(value - mean) / std` if initialized and std > epsilon, else value unchanged - pub fn normalize(&self, value: f64) -> f64 { - // Need at least one sample to have valid statistics - if !self.initialized { - return value; - } - - let std = self.variance.sqrt(); - - // Avoid division by zero for constant values - if std < self.epsilon { - return value; - } - - (value - self.mean) / std - } - - /// Get current mean and standard deviation - pub fn get_stats(&self) -> (f64, f64) { - if !self.initialized { - return (0.0, 0.0); - } - - let std = self.variance.sqrt(); - (self.mean, std) - } - - /// Get count of values seen (backward compatibility) - /// - /// # Note - /// EMA doesn't track sample count. Returns `u64::MAX` to indicate - /// "many samples" for backward compatibility with code that calls this method. - pub const fn count(&self) -> u64 { - if self.initialized { - u64::MAX - } else { - 0 - } - } -} - -impl Default for RewardNormalizer { - fn default() -> Self { - Self::new() - } -} - /// Differential Sharpe Ratio (DSR) for incremental reward shaping. /// /// Implements the Moody & Saffell (2001) formula for online Sharpe ratio @@ -290,6 +137,12 @@ impl DifferentialSharpeRatio { } /// Configuration for reward function +/// +/// NOTE: Reward normalization and DSR are now computed entirely in the GPU +/// experience-collection kernel (`experience_kernels.cu`). The CPU-side +/// `RewardNormalizer` and `use_dsr` toggle have been removed. The composite +/// reward weights (`w_dsr`, `w_pnl`, `w_dd`, `w_idle`, etc.) live in +/// `DQNHyperparameters` and are passed directly to the kernel launch args. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct RewardConfig { /// Weight for P&L component @@ -298,16 +151,10 @@ pub struct RewardConfig { pub risk_weight: Decimal, /// Weight for transaction cost penalty pub cost_weight: Decimal, - /// Weight for hold reward (to reduce over-trading) - pub hold_reward: Decimal, /// Price movement threshold for dynamic HOLD reward (as fraction, e.g., 0.02 = 2%) pub movement_threshold: Decimal, - /// Weight for HOLD action penalty during high volatility (negative value applied) - pub hold_penalty_weight: Decimal, /// Weight for diversity penalty (negative value to penalize low entropy, -0.1 default) pub diversity_weight: Decimal, - /// Enable reward normalization (default: true) - Bug #17 fix - pub enable_normalization: bool, /// Use percentage-based P&L instead of absolute dollar changes (default: true) - Bug #17 fix pub use_percentage_pnl: bool, /// Circuit breaker configuration for risk management @@ -320,9 +167,6 @@ pub struct RewardConfig { pub sharpe_weight: Decimal, /// Rolling window size for Sharpe ratio calculation (WAVE 26 P1.3) pub sharpe_window: usize, - /// Enable Differential Sharpe Ratio (DSR) reward shaping (Moody & Saffell 2001). - /// When true, DSR replaces the EMA normalizer + risk-adjusted division pipeline. - pub use_dsr: bool, /// EMA decay rate for DSR calculation, clamped to [0.0001, 0.1]. /// Smaller values = longer lookback window. Default: 0.01 (~100-step window). pub dsr_eta: f64, @@ -338,18 +182,14 @@ impl Default for RewardConfig { pnl_weight: Decimal::ONE, risk_weight: Decimal::try_from(0.1).unwrap_or(Decimal::ZERO), cost_weight: Decimal::ONE, // Bug #2 fix: Apply transaction costs at full weight (was 0.05) - hold_reward: Decimal::try_from(0.001).unwrap_or(Decimal::ZERO), movement_threshold: Decimal::try_from(0.01).unwrap_or(Decimal::ZERO), // 1% matches data distribution - hold_penalty_weight: Decimal::try_from(0.01).unwrap_or(Decimal::ZERO), // 1% default penalty - diversity_weight: Decimal::try_from(-0.1).unwrap_or(Decimal::ZERO), // -0.1 default (100x stronger than hold_reward) - enable_normalization: false, // Disabled: clipping destroys economic signal magnitude + diversity_weight: Decimal::try_from(-0.1).unwrap_or(Decimal::ZERO), use_percentage_pnl: true, // Bug #17 fix: scale-invariant percentage returns circuit_breaker_config: CircuitBreakerConfig::default(), triple_barrier_profit_bonus: Decimal::try_from(0.5).unwrap_or(Decimal::ZERO), // 50% bonus for hitting profit target triple_barrier_stop_penalty: Decimal::try_from(0.5).unwrap_or(Decimal::ZERO), // 50% penalty for hitting stop loss sharpe_weight: Decimal::try_from(0.3).unwrap_or(Decimal::ZERO), // WAVE 26 P1.3: 30% weight for Sharpe ratio sharpe_window: 20, // WAVE 26 P1.3: 20-step rolling window for Sharpe calculation - use_dsr: false, // DSR disabled by default (opt-in via hyperopt) dsr_eta: 0.01, // ~100-step effective EMA window initial_capital: 100_000.0, } @@ -368,11 +208,10 @@ impl RewardConfig { /// /// MANDATORY VALIDATION to prevent gradient explosion: /// - `use_percentage_pnl` MUST be true (otherwise Q-values explode to ±50,000) - /// - Warns if normalization disabled (suboptimal but not fatal) /// /// ## Root Cause - /// Absolute rewards (±2000) cause steady-state Q-values of ±53,476 - /// Percentage rewards (±0.02) cause steady-state Q-values of ±0.5 + /// Absolute rewards (+-2000) cause steady-state Q-values of +-53,476 + /// Percentage rewards (+-0.02) cause steady-state Q-values of +-0.5 /// This 100,000x difference is the root cause of gradient explosion. pub fn validate(&self) -> Result<(), MLError> { // MANDATORY VALIDATION: use_percentage_pnl MUST be true @@ -390,14 +229,6 @@ impl RewardConfig { ))); } - // Warn if normalization disabled (suboptimal but not fatal) - if !self.enable_normalization { - tracing::warn!( - "Reward normalization disabled. This increases gradient variance. \ - Consider enabling with clip_range=±3.0 for better stability." - ); - } - Ok(()) } } @@ -406,12 +237,9 @@ impl RewardConfig { #[derive(Debug, Default)] pub struct RewardConfigBuilder { pnl_weight: Option, - hold_penalty_weight: Option, activity_bonus_weight: Option, use_percentage_pnl: Option, - enable_normalization: Option, circuit_breaker_config: Option, - use_dsr: Option, dsr_eta: Option, } @@ -421,11 +249,6 @@ impl RewardConfigBuilder { self } - pub const fn hold_penalty_weight(mut self, weight: f64) -> Self { - self.hold_penalty_weight = Some(weight); - self - } - pub const fn activity_bonus_weight(mut self, weight: f64) -> Self { self.activity_bonus_weight = Some(weight); self @@ -436,21 +259,11 @@ impl RewardConfigBuilder { self } - pub const fn enable_normalization(mut self, enable: bool) -> Self { - self.enable_normalization = Some(enable); - self - } - pub const fn circuit_breaker_config(mut self, config: CircuitBreakerConfig) -> Self { self.circuit_breaker_config = Some(config); self } - pub const fn use_dsr(mut self, enable: bool) -> Self { - self.use_dsr = Some(enable); - self - } - pub const fn dsr_eta(mut self, eta: f64) -> Self { self.dsr_eta = Some(eta); self @@ -462,20 +275,15 @@ impl RewardConfigBuilder { .map_err(|e| MLError::InvalidInput(format!("Invalid pnl_weight: {}", e)))?, risk_weight: Decimal::try_from(0.1).unwrap_or(Decimal::ZERO), cost_weight: Decimal::ONE, // Bug #2 fix: Apply transaction costs at full weight (was 0.05) - hold_reward: Decimal::try_from(0.001).unwrap_or(Decimal::ZERO), movement_threshold: Decimal::try_from(0.01).unwrap_or(Decimal::ZERO), - hold_penalty_weight: Decimal::try_from(self.hold_penalty_weight.unwrap_or(0.01)) - .map_err(|e| MLError::InvalidInput(format!("Invalid hold_penalty_weight: {}", e)))?, diversity_weight: Decimal::try_from(self.activity_bonus_weight.unwrap_or(0.0)) .map_err(|e| MLError::InvalidInput(format!("Invalid activity_bonus_weight: {}", e)))?, - enable_normalization: self.enable_normalization.unwrap_or(true), use_percentage_pnl: self.use_percentage_pnl.unwrap_or(true), circuit_breaker_config: self.circuit_breaker_config.unwrap_or_default(), triple_barrier_profit_bonus: Decimal::try_from(0.5).unwrap_or(Decimal::ZERO), triple_barrier_stop_penalty: Decimal::try_from(0.5).unwrap_or(Decimal::ZERO), sharpe_weight: Decimal::try_from(0.3).unwrap_or(Decimal::ZERO), sharpe_window: 100, - use_dsr: self.use_dsr.unwrap_or(false), dsr_eta: self.dsr_eta.unwrap_or(0.01), initial_capital: 100_000.0, }) @@ -553,20 +361,24 @@ fn calculate_entropy(recent_actions: &[FactoredAction]) -> Decimal { } /// Reward function for `DQN` training +/// +/// NOTE: The primary reward path now runs in the GPU experience-collection +/// kernel. This CPU-side `RewardFunction` is retained for: +/// - DSR state sync between GPU and CPU (logging / checkpointing) +/// - Epoch-level state resets +/// - CPU-fallback reward calculation (testing / evaluation) #[derive(Debug)] pub struct RewardFunction { /// Configuration config: RewardConfig, /// Previous rewards for tracking (`VecDeque` for O(1) front removal) reward_history: VecDeque, - /// Optional reward normalizer (None = disabled, Some = enabled) - normalizer: Option, /// Enable debug logging (`REWARD_DEBUG`, gradient norms, etc.) debug_logging: bool, /// Rolling buffer of returns for Sharpe ratio calculation (WAVE 26 P1.3) returns_buffer: std::collections::VecDeque, - /// Optional Differential Sharpe Ratio calculator (None = disabled) - dsr: Option, + /// Differential Sharpe Ratio calculator (always enabled) + dsr: DifferentialSharpeRatio, } impl RewardFunction { @@ -590,13 +402,11 @@ impl RewardFunction { // Fix #2: Validate config on construction to prevent gradient explosion config.validate()?; - let normalizer = config.enable_normalization.then(RewardNormalizer::new); - let dsr = config.use_dsr.then(|| DifferentialSharpeRatio::new(config.dsr_eta)); + let dsr = DifferentialSharpeRatio::new(config.dsr_eta); Ok(Self { config: config.clone(), reward_history: VecDeque::new(), - normalizer, debug_logging, returns_buffer: std::collections::VecDeque::with_capacity(config.sharpe_window), dsr, @@ -678,8 +488,9 @@ impl RewardFunction { - self.config.cost_weight * cost_penalty }, TradingAction::Hold => { - // Dynamic HOLD reward based on price movement - self.calculate_hold_reward(current_state, next_state)? + // Neutral hold reward: 0.0 (no penalty, no bonus). + // Idle penalty is now in the GPU composite reward kernel (w_idle). + Decimal::ZERO }, }; @@ -688,74 +499,20 @@ impl RewardFunction { let entropy = calculate_entropy(recent_actions); let entropy_threshold = Decimal::try_from(0.3).unwrap_or(Decimal::ZERO); let diversity_bonus = if entropy < entropy_threshold { - // DSR mode: scale penalty proportional to trading signal (~0.0005) - // Legacy mode: -0.1 (500x larger — known to dominate reward signal) - if self.config.use_dsr { - Decimal::try_from(-0.0005).unwrap_or(Decimal::ZERO) - } else { - self.config.diversity_weight // -0.1 (penalty for low diversity) - } + // Scale penalty proportional to trading signal (~0.0005) + Decimal::try_from(-0.0005).unwrap_or(Decimal::ZERO) } else { Decimal::ZERO // No penalty for balanced actions }; let pnl_base_reward = base_reward + diversity_bonus; - let final_reward_f64: f64 = if self.config.use_dsr { - // DSR path: skip Sharpe blend and EMA normalizer entirely. - // Pass raw PnL + diversity through DSR transform for dense risk-adjusted reward. - let raw_pnl: f64 = pnl_base_reward.try_into() - .map_err(|e| MLError::InvalidInput(format!("Reward conversion failed: {}", e)))?; + // DSR path: pass raw PnL + diversity through DSR transform for dense + // risk-adjusted reward. Normalization lives in the GPU kernel now. + let raw_pnl: f64 = pnl_base_reward.try_into() + .map_err(|e| MLError::InvalidInput(format!("Reward conversion failed: {}", e)))?; - if let Some(ref mut dsr) = self.dsr { - dsr.step(raw_pnl) - } else { - // Fallback: scale raw PnL to reasonable range - raw_pnl * 100.0 - } - } else { - // Legacy path: Sharpe blend + optional EMA normalization (unchanged) - let pnl_normalized_f64: f64 = ((next_state.portfolio_features.first().unwrap_or(&100000.0) - - current_state.portfolio_features.first().unwrap_or(&100000.0)) - / current_state.portfolio_features.first().unwrap_or(&100000.0)) as f64; - - // Update returns buffer for Sharpe calculation - self.returns_buffer.push_back(pnl_normalized_f64); - if self.returns_buffer.len() > self.config.sharpe_window { - self.returns_buffer.pop_front(); - } - - // Calculate Sharpe component (mean return / std dev of returns) - let sharpe_component = if self.returns_buffer.len() > 1 { - let mean: f64 = self.returns_buffer.iter().sum::() / self.returns_buffer.len() as f64; - let variance: f64 = self.returns_buffer.iter() - .map(|r| (r - mean).powi(2)) - .sum::() / self.returns_buffer.len() as f64; - let std_dev = variance.sqrt(); - if std_dev > 1e-8 { mean / std_dev } else { 0.0 } - } else { - 0.0 - }; - let sharpe_decimal = Decimal::try_from(sharpe_component) - .unwrap_or(Decimal::ZERO); - - // WAVE 26 P1.3: Blend PnL and Sharpe components - let sharpe_weight = self.config.sharpe_weight; - let pnl_weight_adjusted = Decimal::ONE - sharpe_weight; - let final_reward = pnl_base_reward * pnl_weight_adjusted + sharpe_decimal * sharpe_weight; - - let final_reward_f64_inner: f64 = final_reward.try_into() - .map_err(|e| MLError::InvalidInput(format!("Reward conversion failed: {}", e)))?; - - // Apply normalization if enabled (Bug #17 fix) - if let Some(normalizer) = &mut self.normalizer { - let norm = normalizer.normalize(final_reward_f64_inner); - normalizer.update(final_reward_f64_inner); - norm.clamp(-3.0, 3.0) - } else { - final_reward_f64_inner - } - }; + let final_reward_f64: f64 = self.dsr.step(raw_pnl); // Convert back to Decimal for storage let final_reward_decimal = Decimal::try_from(final_reward_f64) @@ -1048,48 +805,6 @@ impl RewardFunction { cost_penalty } - /// Calculate dynamic HOLD reward based on log return volatility - /// - /// Strategy: Penalize holding during high-velocity price movements. - /// - Low volatility (|`next_log_return`| < threshold): Grant positive reward - /// - High volatility (|`next_log_return`| >= threshold): Apply negative penalty - /// - /// # BUG FIX: Hold Penalty Unit Mismatch (Phase 1) - /// **Root Cause**: Hold penalty was applied as raw scalar (0.5-2.0), while transaction - /// costs are in percentage scale (0.0005-0.0015 = 0.05-0.15%), creating 333-4000x mismatch. - /// - /// **Solution**: Scale `hold_penalty_weight` by 1/10000 to convert to percentage units: - /// - Config value: 0.5 (raw scalar from hyperopt search space) - /// - Scaled value: 0.5 / 10000 = 0.00005 (5 basis points = 0.005%) - /// - Transaction cost range: 0.0005-0.0015 (5-15 basis points = 0.05-0.15%) - /// - New ratio: `hold_penalty` / `tx_cost` = 0.00005 / 0.001 = 0.05 (5%, reasonable) - /// - /// **Why 1/10000?** - /// - Preserves hyperopt search space (0.5-2.0 remains interpretable) - /// - Aligns magnitude with transaction costs (both in basis points) - /// - Prevents hold penalty from dominating reward signal (was 90x too strong) - /// - /// **Expected Impact**: - /// - Higher penalties → less frequent trading (reduces turnover) - /// - Lower penalties → more active strategies (increases exploration) - /// - /// Note: `price_features`[0] contains log returns (normalized price volatility measure), - /// not raw prices. Using log returns directly is zero-safe and mathematically correct. - /// The absolute value of the log return measures the magnitude of the current price - /// movement (velocity), analogous to the original price change percentage logic. - #[allow(clippy::unnecessary_wraps)] - const fn calculate_hold_reward( - &self, - _current_state: &TradingState, - _next_state: &TradingState, - ) -> Result { - // C3 FIX: Neutral hold reward. - // Hold = 0.0 (no penalty, no bonus). The diversity penalty in the hyperopt - // objective already catches "always hold" degenerate trials. Penalizing hold - // biases Q-values toward unnecessary trading, inflating trade counts. - Ok(Decimal::ZERO) - } - /// Get average reward over recent history pub fn get_average_reward(&self, window: usize) -> Decimal { let window = window.min(self.reward_history.len()); @@ -1105,11 +820,8 @@ impl RewardFunction { /// Reset the DSR calculator state for episode boundaries. /// /// Call this between episodes to prevent cross-episode EMA leakage. - /// No-op if DSR is not enabled. pub const fn reset_dsr(&mut self) { - if let Some(dsr) = &mut self.dsr { - dsr.reset(); - } + self.dsr.reset(); } /// Synchronize CPU DSR state from GPU `epoch_state` readback. @@ -1117,23 +829,16 @@ impl RewardFunction { /// After GPU experience collection, the kernel's DSR EMA accumulators /// are authoritative. This method copies them into the CPU-side DSR /// calculator for logging, checkpointing, and cross-epoch warm-start. - /// - /// No-op if DSR is not enabled. pub const fn sync_dsr_from_gpu(&mut self, ema_a: f64, ema_b: f64) { - if let Some(ref mut dsr) = self.dsr { - dsr.sync_from_gpu(ema_a, ema_b); - } + self.dsr.sync_from_gpu(ema_a, ema_b); } /// Reset all epoch-level state for reward stationarity between epochs. - /// Resets DSR calculator, returns buffer, normalizer, and reward history. + /// Resets DSR calculator, returns buffer, and reward history. pub fn reset_epoch_state(&mut self) { self.reset_dsr(); self.returns_buffer.clear(); self.reward_history.clear(); - if let Some(normalizer) = &mut self.normalizer { - *normalizer = RewardNormalizer::new(); - } } /// Get reward statistics @@ -1587,11 +1292,9 @@ mod tests { #[test] fn test_dsr_config_roundtrip() -> anyhow::Result<()> { let config = RewardConfig::builder() - .use_dsr(true) .dsr_eta(0.02) .build()?; - assert!(config.use_dsr, "use_dsr should be true"); assert!( (config.dsr_eta - 0.02).abs() < f64::EPSILON, "dsr_eta should be 0.02, got {}", @@ -1600,7 +1303,6 @@ mod tests { // Verify defaults when not set let default_config = RewardConfig::builder().build()?; - assert!(!default_config.use_dsr, "use_dsr should default to false"); assert!( (default_config.dsr_eta - 0.01).abs() < f64::EPSILON, "dsr_eta should default to 0.01, got {}", diff --git a/crates/ml/src/trainers/dqn/config.rs b/crates/ml/src/trainers/dqn/config.rs index 29bf05663..ed4e2804e 100644 --- a/crates/ml/src/trainers/dqn/config.rs +++ b/crates/ml/src/trainers/dqn/config.rs @@ -839,7 +839,8 @@ pub struct DQNHyperparameters { pub use_double_dqn: bool, /// Gradient clipping max norm (None = disabled) pub gradient_clip_norm: Option, - /// HOLD action penalty weight (penalizes holding during large price movements) + /// Deprecated: was HOLD action penalty weight, replaced by `w_idle` in GPU composite reward. + /// Kept for backward compatibility with hyperopt adapters. Not used in reward computation. pub hold_penalty_weight: f64, /// Price movement threshold for HOLD penalty (as fraction, e.g., 0.02 = 2%) pub movement_threshold: f64, @@ -1127,8 +1128,8 @@ pub struct DQNHyperparameters { /// Range [1.1, 2.0]: 1.1 = 10% margin above costs, 2.0 = 100% margin. pub minimum_profit_factor: f64, - // Differential Sharpe Ratio (DSR) — replaces EMA normalizer + risk-adjusted division - /// Enable DSR reward shaping (Moody & Saffell 2001) + // Differential Sharpe Ratio (DSR) — always enabled in GPU composite reward kernel. + /// Deprecated: DSR is always enabled. Kept for backward compatibility with hyperopt adapters. pub use_dsr: bool, /// DSR EMA decay rate (0.001=slow, 0.05=fast). Tunable in hyperopt. pub dsr_eta: f64, diff --git a/crates/ml/src/trainers/dqn/trainer/constructor.rs b/crates/ml/src/trainers/dqn/trainer/constructor.rs index a85f8a85d..513e7164a 100644 --- a/crates/ml/src/trainers/dqn/trainer/constructor.rs +++ b/crates/ml/src/trainers/dqn/trainer/constructor.rs @@ -354,26 +354,20 @@ impl DQNTrainer { ); // Initialize reward function with hyperparameter-driven configuration - // WAVE 10-A9 FIX: Wire hold_penalty_weight from hyperparameters to RewardConfig - // BUG #17 FIX: Add normalization and percentage-based P&L (enabled by default) + // Reward normalization and DSR now live in the GPU composite reward kernel. let reward_config = RewardConfig { pnl_weight: Decimal::ONE, risk_weight: Decimal::try_from(0.1).unwrap_or(Decimal::ZERO), cost_weight: Decimal::ONE, // Bug #2 fix: 100% transaction cost weight (was 0.05, 20x too low) - hold_reward: Decimal::ZERO, // Flat position = no edge = zero reward (was +0.001, 20x trade PnL) movement_threshold: Decimal::try_from(hyperparams.movement_threshold) .unwrap_or(Decimal::ZERO), - hold_penalty_weight: Decimal::try_from(hyperparams.hold_penalty_weight) - .unwrap_or(Decimal::ZERO), // CRITICAL FIX diversity_weight: Decimal::try_from(-0.1).unwrap_or(Decimal::ZERO), - enable_normalization: !hyperparams.use_dsr, // DSR replaces EMA normalizer use_percentage_pnl: true, // Bug #17: Use percentage returns for scale-invariance circuit_breaker_config: CircuitBreakerConfig::default(), triple_barrier_profit_bonus: Decimal::try_from(0.5).unwrap_or(Decimal::ZERO), triple_barrier_stop_penalty: Decimal::try_from(0.5).unwrap_or(Decimal::ZERO), sharpe_weight: Decimal::ZERO, // WAVE 26 P1.3: Disabled by default sharpe_window: 20, // WAVE 26 P1.3: Standard 20-period window - use_dsr: hyperparams.use_dsr, dsr_eta: hyperparams.dsr_eta, initial_capital: hyperparams.initial_capital as f64, }; diff --git a/crates/ml/src/trainers/dqn/trainer/training_loop.rs b/crates/ml/src/trainers/dqn/trainer/training_loop.rs index 906649a9f..b1ab210bc 100644 --- a/crates/ml/src/trainers/dqn/trainer/training_loop.rs +++ b/crates/ml/src/trainers/dqn/trainer/training_loop.rs @@ -297,18 +297,11 @@ impl DQNTrainer { self.metrics_aggregator.reset(); self.pnl_history.clear(); self.reward_fn.reset_epoch_state(); - - if self.hyperparams.use_dsr { - self.portfolio_tracker.reset(); - } + self.portfolio_tracker.reset(); // GPU-persistent epoch state: set reset flags instead of CPU state mutation. if let Some(ref mut collector) = self.gpu_experience_collector { - let mut flags: u32 = 0; - if self.hyperparams.use_dsr { - flags |= 1; // reset portfolio - flags |= 2; // reset DSR normalizer - } + let flags: u32 = 1 | 2; // reset portfolio + DSR normalizer (always enabled) collector.set_reset_flags(flags); } } @@ -856,18 +849,16 @@ impl DQNTrainer { // Sync GPU DSR EMA state back to CPU for logging/checkpointing. // The GPU kernel's dsr_step() maintains its own accumulators in epoch_state[5..6]. // After experience collection, read them back so the CPU RewardFunction stays current. - if self.hyperparams.use_dsr { - match collector.read_epoch_dsr_state() { - Ok((dsr_a, dsr_b)) => { - self.reward_fn.sync_dsr_from_gpu(dsr_a as f64, dsr_b as f64); - debug!( - "DSR GPU->CPU sync: ema_return={:.6}, ema_return_sq={:.6}", - dsr_a, dsr_b - ); - } - Err(e) => { - debug!("DSR epoch_state readback failed (non-fatal): {e}"); - } + match collector.read_epoch_dsr_state() { + Ok((dsr_a, dsr_b)) => { + self.reward_fn.sync_dsr_from_gpu(dsr_a as f64, dsr_b as f64); + debug!( + "DSR GPU->CPU sync: ema_return={:.6}, ema_return_sq={:.6}", + dsr_a, dsr_b + ); + } + Err(e) => { + debug!("DSR epoch_state readback failed (non-fatal): {e}"); } }