diff --git a/crates/ml-dqn/src/reward.rs b/crates/ml-dqn/src/reward.rs index 1cf3e8086..a6ec67b68 100644 --- a/crates/ml-dqn/src/reward.rs +++ b/crates/ml-dqn/src/reward.rs @@ -155,8 +155,6 @@ pub struct RewardConfig { pub movement_threshold: Decimal, /// Weight for diversity penalty (negative value to penalize low entropy, -0.1 default) pub diversity_weight: Decimal, - /// 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 pub circuit_breaker_config: CircuitBreakerConfig, /// Triple barrier profit target scaling factor (1.0 = 100% bonus, 0.5 = 50% bonus) @@ -184,7 +182,6 @@ impl Default for RewardConfig { cost_weight: Decimal::ONE, // Bug #2 fix: Apply transaction costs at full weight (was 0.05) movement_threshold: Decimal::try_from(0.01).unwrap_or(Decimal::ZERO), // 1% matches data distribution 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 @@ -202,25 +199,12 @@ impl RewardConfig { RewardConfigBuilder::default() } - /// Validate configuration to prevent gradient explosion + /// Validate configuration. /// - /// # Fix #2: Force Percentage Rewards + Validation - /// - /// MANDATORY VALIDATION to prevent gradient explosion: - /// - `use_percentage_pnl` MUST be true (otherwise Q-values explode to ±50,000) - /// - /// ## 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 - /// This 100,000x difference is the root cause of gradient explosion. + /// Percentage-based P&L is unconditional (see `calculate_pnl_reward`); + /// absolute-dollar mode was removed because it caused gradient explosion + /// (Q-values ±50,000 vs. ±0.5 with percentage returns — 100,000x ratio). pub fn validate(&self) -> Result<(), MLError> { - // MANDATORY VALIDATION: use_percentage_pnl MUST be true - if !self.use_percentage_pnl { - return Err(MLError::ConfigError("FATAL: use_percentage_pnl=false causes gradient explosion. \ - Q-values explode to ±50,000, gradients to millions. \ - MUST use percentage_pnl=true for numerical stability.".to_owned())); - } - // Validate initial_capital is positive (needed for denormalization) if self.initial_capital <= 0.0 { return Err(MLError::ConfigError(format!( @@ -238,7 +222,6 @@ impl RewardConfig { pub struct RewardConfigBuilder { pnl_weight: Option, activity_bonus_weight: Option, - use_percentage_pnl: Option, circuit_breaker_config: Option, dsr_eta: Option, } @@ -254,11 +237,6 @@ impl RewardConfigBuilder { self } - pub const fn use_percentage_pnl(mut self, enable: bool) -> Self { - self.use_percentage_pnl = Some(enable); - self - } - pub const fn circuit_breaker_config(mut self, config: CircuitBreakerConfig) -> Self { self.circuit_breaker_config = Some(config); self @@ -278,7 +256,6 @@ impl RewardConfigBuilder { movement_threshold: Decimal::try_from(0.01).unwrap_or(Decimal::ZERO), diversity_weight: Decimal::try_from(self.activity_bonus_weight.unwrap_or(0.0)) .map_err(|e| MLError::InvalidInput(format!("Invalid activity_bonus_weight: {}", e)))?, - 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), @@ -385,7 +362,7 @@ impl RewardFunction { /// Create a new reward function with debug logging disabled /// /// # Errors - /// Returns `MLError::ConfigError` if config validation fails (e.g., `use_percentage_pnl=false`) + /// Returns `MLError::ConfigError` if config validation fails (e.g., non-positive `initial_capital`). pub fn new(config: RewardConfig) -> Result { Self::new_with_debug(config, false) } @@ -397,7 +374,7 @@ impl RewardFunction { /// * `debug_logging` - Enable debug logging (`REWARD_DEBUG`, gradient norms, etc.) /// /// # Errors - /// Returns `MLError::ConfigError` if config validation fails (e.g., `use_percentage_pnl=false`) + /// Returns `MLError::ConfigError` if config validation fails (e.g., non-positive `initial_capital`). pub fn new_with_debug(config: RewardConfig, debug_logging: bool) -> Result { // Fix #2: Validate config on construction to prevent gradient explosion config.validate()?; @@ -622,55 +599,33 @@ impl RewardFunction { Decimal::try_from((next_normalized + 1.0) * initial_capital) .unwrap_or(default_val); - // Bug #17 Fix: Use percentage-based P&L or absolute dollar change - let pnl_reward = if self.config.use_percentage_pnl { - // Percentage-based: pct_return = (next - current) / current - // Avoid division by zero - if current_value <= Decimal::ZERO { - tracing::debug!("Current portfolio value <= 0, using fallback reward 0.0"); - Decimal::ZERO - } else { - let pct_return = (next_value - current_value) / current_value; - - // REWARD_DEBUG: Diagnostic logging to confirm percentage-based rewards - // Expected: pct_return ±0.02 (±2% per step) - // If broken: pct_return would be 100x-1000x larger (±20 to ±2000) - if self.debug_logging { - tracing::info!( - "REWARD_DEBUG: use_percentage_pnl={}, current_value={:.2}, next_value={:.2}, pct_return={:.6}", - self.config.use_percentage_pnl, - current_value, - next_value, - pct_return - ); - } - - // Scale PnL by agent's target exposure so the reward reflects the - // agent's intended position: - // Long100 (+1.0): full market return - // Short100 (-1.0): inverted market return (profit when market drops) - // Flat (0.0): zero PnL (no skin in the game) - // Long50/Short50 (±0.5): half-size position - let exposure_decimal = Decimal::try_from(target_exposure) - .unwrap_or(Decimal::ONE); - pct_return * exposure_decimal - } + // Percentage-based P&L (Bug #17 fix — scale-invariant returns). + // Absolute-dollar mode was removed: Q-values exploded ±50,000 vs. ±0.5. + // Avoid division by zero when portfolio_value <= 0. + let pnl_reward = if current_value <= Decimal::ZERO { + tracing::debug!("Current portfolio value <= 0, using fallback reward 0.0"); + Decimal::ZERO } else { - // Absolute dollar change (original implementation) - let pnl_change = next_value - current_value; + let pct_return = (next_value - current_value) / current_value; - // REWARD_DEBUG: Diagnostic logging if absolute rewards are used (BUG!) if self.debug_logging { - tracing::warn!( - "REWARD_DEBUG: using ABSOLUTE rewards! use_percentage_pnl={}, pnl_change={:.2}, scaled_reward={:.6}", - self.config.use_percentage_pnl, - pnl_change, - pnl_change / Decimal::try_from(10000.0).unwrap_or(Decimal::ONE) + tracing::info!( + "REWARD_DEBUG: current_value={:.2}, next_value={:.2}, pct_return={:.6}", + current_value, + next_value, + pct_return ); } - // Scale down by 10,000 to keep reward magnitude reasonable - pnl_change / Decimal::try_from(10000.0).unwrap_or(Decimal::ONE) + // Scale PnL by agent's target exposure so the reward reflects the + // agent's intended position: + // Long100 (+1.0): full market return + // Short100 (-1.0): inverted market return (profit when market drops) + // Flat (0.0): zero PnL (no skin in the game) + // Long50/Short50 (±0.5): half-size position + let exposure_decimal = Decimal::try_from(target_exposure) + .unwrap_or(Decimal::ONE); + pct_return * exposure_decimal }; Ok(pnl_reward) diff --git a/crates/ml/src/trainers/dqn/trainer/constructor.rs b/crates/ml/src/trainers/dqn/trainer/constructor.rs index f37c91b6e..96840169a 100644 --- a/crates/ml/src/trainers/dqn/trainer/constructor.rs +++ b/crates/ml/src/trainers/dqn/trainer/constructor.rs @@ -345,7 +345,6 @@ impl DQNTrainer { movement_threshold: Decimal::try_from(hyperparams.movement_threshold) .unwrap_or(Decimal::ZERO), diversity_weight: Decimal::try_from(-0.1).unwrap_or(Decimal::ZERO), - 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),