cleanup(fflag): remove use_percentage_pnl — always on — [FFLAG-001]
The field was gated by validate() to always be true (absolute-dollar mode caused Q-value explosion ±50,000). All call sites already set true. Deleted: - RewardConfig.use_percentage_pnl field + Default init - Absolute-dollar branch in calculate_pnl_reward (dead) - Validation guard against false - RewardConfigBuilder field + builder method + build() init - Constructor call site Percentage-based P&L is now unconditional per feedback_no_feature_flags.md.
This commit is contained in:
jgrusewski
@@ -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<f64>,
|
||||
activity_bonus_weight: Option<f64>,
|
||||
use_percentage_pnl: Option<bool>,
|
||||
circuit_breaker_config: Option<CircuitBreakerConfig>,
|
||||
dsr_eta: Option<f64>,
|
||||
}
|
||||
@@ -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, MLError> {
|
||||
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<Self, MLError> {
|
||||
// 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)
|
||||
|
||||
@@ -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),
|
||||
|
||||
Reference in New Issue
Block a user