From e906ab96d12c0abe8b64b4348521c56e95f1d36a Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Thu, 23 Apr 2026 09:24:17 +0200 Subject: [PATCH] fix(risk): return Option for insufficient-samples Sharpe/Sortino MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit services/trading_service/src/services/risk.rs compute_sharpe_ratio / compute_sortino_ratio previously returned `0.0` as an "insufficient-samples" fallback. Plain zero is indistinguishable from a legitimate "Sharpe happened to equal 0" result; callers gating on `sharpe > threshold` silently pass/fail on the fallback, and downstream monitoring logs it as a real value. Now: returns `Option` — `Some(value)` for valid samples, `None` when: * returns.len() < MIN_RETURN_OBSERVATIONS (threshold at line 30 unchanged — only the return-shape changes) * daily_std < 1e-12 (degenerate zero-volatility series; ratio mathematically undefined, not zero) * downside_count < 2 (Sortino only; too few sub-risk-free observations to estimate downside variance) * downside_dev < 1e-12 (Sortino only) Production caller (get_risk_metrics, the only call site) updated: * On Some/Some: debug-log as before * On either None: WARN-log with `[insufficient-samples]` tag, formatting each ratio as "None" or "{:.4}" so operators can distinguish missing data from a real zero * Protobuf RiskMetrics { sharpe_ratio, sortino_ratio } is a non-optional `double` in risk.proto — export `f64::NAN` rather than a fake 0, so consumers can detect the undefined case without silently failing threshold gates. Tests: each function now has happy-path `Some(_)`, insufficient-samples `None`, and empty-input `None` cases; existing `_zero_volatility` / `_no_downside` tests re-asserted against `None` (previously asserted against 0.0). compute_volatility and compute_current_drawdown retain `f64` return — their 0.0 outputs are legitimate (no position → 0% drawdown; constant returns → 0% volatility). Callers touched: - services/trading_service/src/services/risk.rs:570-596 (get_risk_metrics — only production caller) - services/trading_service/src/services/risk.rs:1585-1671 (tests) Cross-file grep (compute_sharpe|compute_sortino|RiskServiceImpl::) confirms no other callers in crates/, services/, bin/. position_manager.rs line 772 references the function in a comment only. Compile: SQLX_OFFLINE=true CARGO_INCREMENTAL=0 cargo check --workspace --tests — clean on risk.rs (pre-existing ml-crate warnings unchanged). Tests: cargo test -p trading-service --lib risk — 59 passed, 0 failed, including 2 new empty-input tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- services/trading_service/src/services/risk.rs | 124 ++++++++++++++---- 1 file changed, 97 insertions(+), 27 deletions(-) diff --git a/services/trading_service/src/services/risk.rs b/services/trading_service/src/services/risk.rs index e0779ac85..f77826528 100644 --- a/services/trading_service/src/services/risk.rs +++ b/services/trading_service/src/services/risk.rs @@ -27,7 +27,11 @@ pub struct RiskServiceImpl { const RISK_FREE_RATE_ANNUAL: f64 = 0.05; /// Minimum number of return observations required for ratio calculations. -/// Below this threshold, Sharpe/Sortino return 0.0 to avoid noisy estimates. +/// Below this threshold, `compute_sharpe_ratio` / `compute_sortino_ratio` +/// return `None` to avoid noisy estimates. Callers MUST distinguish +/// "insufficient samples" from a legitimate zero-valued ratio; returning +/// a plain `0.0` would silently pass/fail threshold gates (e.g. +/// `sharpe > 1.0`) and pollute downstream monitoring with fake zeros. const MIN_RETURN_OBSERVATIONS: usize = 5; impl RiskServiceImpl { @@ -226,10 +230,18 @@ impl RiskServiceImpl { /// Compute annualized Sharpe ratio from a return series. /// /// Sharpe = (mean_return - risk_free_daily) / std_dev, annualized. - /// Returns 0.0 if insufficient data or zero volatility. - fn compute_sharpe_ratio(returns: &[f64]) -> f64 { + /// + /// Returns `None` in three distinct cases — callers MUST handle each + /// explicitly (conservative gating, `[insufficient-samples]`-tagged + /// logging, NaN metric export): + /// * `returns.len() < MIN_RETURN_OBSERVATIONS` — too few samples. + /// * `returns.len() < 2` — sample variance is undefined. + /// * `daily_std < 1e-12` — the ratio is mathematically undefined + /// (degenerate zero-volatility series), distinct from a legitimate + /// Sharpe that happens to equal 0. + fn compute_sharpe_ratio(returns: &[f64]) -> Option { if returns.len() < MIN_RETURN_OBSERVATIONS { - return 0.0; + return None; } let n = returns.len() as f64; @@ -238,24 +250,31 @@ impl RiskServiceImpl { let daily_std = variance.sqrt(); if daily_std < 1e-12 { - return 0.0; + return None; } let daily_rf = RISK_FREE_RATE_ANNUAL / 252.0; let excess_return = mean_return - daily_rf; // Annualized Sharpe - (excess_return / daily_std) * 252_f64.sqrt() + Some((excess_return / daily_std) * 252_f64.sqrt()) } /// Compute annualized Sortino ratio from a return series. /// /// Sortino = (mean_return - risk_free_daily) / downside_deviation, annualized. /// Downside deviation only considers returns below the risk-free rate. - /// Returns 0.0 if insufficient data or zero downside deviation. - fn compute_sortino_ratio(returns: &[f64]) -> f64 { + /// + /// Returns `None` when the ratio cannot be computed — callers MUST + /// handle this explicitly (do NOT substitute a default 0.0, which would + /// be indistinguishable from a legitimate zero-valued Sortino): + /// * `returns.len() < MIN_RETURN_OBSERVATIONS` — too few samples. + /// * `downside_count < 2` — not enough sub-risk-free returns to + /// estimate downside variance. + /// * `downside_dev < 1e-12` — downside deviation is degenerate. + fn compute_sortino_ratio(returns: &[f64]) -> Option { if returns.len() < MIN_RETURN_OBSERVATIONS { - return 0.0; + return None; } let n = returns.len() as f64; @@ -271,18 +290,18 @@ impl RiskServiceImpl { let downside_count = returns.iter().filter(|&&r| r < daily_rf).count(); if downside_count < 2 { - return 0.0; + return None; } let downside_dev = (downside_sq_sum / (downside_count as f64 - 1.0)).sqrt(); if downside_dev < 1e-12 { - return 0.0; + return None; } let excess_return = mean_return - daily_rf; // Annualized Sortino - (excess_return / downside_dev) * 252_f64.sqrt() + Some((excess_return / downside_dev) * 252_f64.sqrt()) } /// Build per-position risk entries using position data and VaR from the risk engine. @@ -549,13 +568,32 @@ impl RiskService for RiskServiceImpl { let num_returns = returns.len(); let volatility = Self::compute_volatility(&returns); - let sharpe_ratio = Self::compute_sharpe_ratio(&returns); - let sortino_ratio = Self::compute_sortino_ratio(&returns); + let sharpe_opt = Self::compute_sharpe_ratio(&returns); + let sortino_opt = Self::compute_sortino_ratio(&returns); - debug!( - "Risk metrics computed: drawdown={:.4}, vol={:.4}, sharpe={:.4}, sortino={:.4}, returns_n={}", - current_drawdown, volatility, sharpe_ratio, sortino_ratio, num_returns - ); + // Export NaN when the ratio is undefined (insufficient samples, + // zero volatility, or too few downside observations). NaN is the + // correct wire-format sentinel for a non-optional `double` in the + // protobuf schema — it is distinguishable from a legitimate 0.0 + // by consumers and will not silently pass/fail threshold gates. + let sharpe_ratio = sharpe_opt.unwrap_or(f64::NAN); + let sortino_ratio = sortino_opt.unwrap_or(f64::NAN); + + match (sharpe_opt, sortino_opt) { + (Some(s), Some(so)) => debug!( + "Risk metrics computed: drawdown={:.4}, vol={:.4}, sharpe={:.4}, sortino={:.4}, returns_n={}", + current_drawdown, volatility, s, so, num_returns + ), + _ => warn!( + "Risk metrics computed [insufficient-samples]: drawdown={:.4}, vol={:.4}, sharpe={}, sortino={}, returns_n={} (min required={})", + current_drawdown, + volatility, + sharpe_opt.map_or_else(|| "None".to_string(), |v| format!("{:.4}", v)), + sortino_opt.map_or_else(|| "None".to_string(), |v| format!("{:.4}", v)), + num_returns, + MIN_RETURN_OBSERVATIONS + ), + } // Build per-position risk entries with VaR contributions let position_risks = self.build_position_risks(&positions).await; @@ -1544,19 +1582,33 @@ mod tests { // ----------------------------------------------------------------------- // 6. compute_sharpe_ratio // ----------------------------------------------------------------------- + #[test] + fn test_sharpe_empty_returns() { + // Empty slice: below MIN_RETURN_OBSERVATIONS => None + let sharpe = RiskServiceImpl::compute_sharpe_ratio(&[]); + assert!(sharpe.is_none(), "Empty returns must yield None, not fake 0.0"); + } + #[test] fn test_sharpe_insufficient_data() { // Fewer than MIN_RETURN_OBSERVATIONS (5) returns let returns = vec![0.01, 0.02, 0.01, -0.01]; let sharpe = RiskServiceImpl::compute_sharpe_ratio(&returns); - assert!((sharpe - 0.0).abs() < 1e-10, "Should return 0.0 for insufficient data"); + assert!( + sharpe.is_none(), + "Should return None for insufficient data (not fake 0.0 — caller must distinguish \ + insufficient-samples from a legitimate zero-valued Sharpe)" + ); } #[test] fn test_sharpe_zero_volatility() { + // Degenerate zero-volatility series: ratio is mathematically undefined, + // MUST return None (not 0.0 — that would be indistinguishable from a + // real Sharpe ratio that happens to equal 0). let returns = vec![0.01; 10]; let sharpe = RiskServiceImpl::compute_sharpe_ratio(&returns); - assert!((sharpe - 0.0).abs() < 1e-10, "Zero volatility should return 0.0 Sharpe"); + assert!(sharpe.is_none(), "Zero-volatility series must yield None, not fake 0.0"); } #[test] @@ -1564,28 +1616,45 @@ mod tests { // Use enough data points with positive mean return let returns = vec![0.01, 0.02, 0.015, 0.005, 0.012, 0.008]; let sharpe = RiskServiceImpl::compute_sharpe_ratio(&returns); - // Mean return is clearly positive and above risk-free => Sharpe should be positive - assert!(sharpe > 0.0, "Sharpe should be positive for consistently positive returns; got {}", sharpe); + // Mean return is clearly positive and above risk-free => Sharpe should be Some(positive) + let value = sharpe.expect("Sharpe should be Some(_) for a well-conditioned return series"); + assert!(value > 0.0, "Sharpe should be positive for consistently positive returns; got {}", value); } // ----------------------------------------------------------------------- // 7. compute_sortino_ratio // ----------------------------------------------------------------------- + #[test] + fn test_sortino_empty_returns() { + // Empty slice: below MIN_RETURN_OBSERVATIONS => None + let sortino = RiskServiceImpl::compute_sortino_ratio(&[]); + assert!(sortino.is_none(), "Empty returns must yield None, not fake 0.0"); + } + #[test] fn test_sortino_insufficient_data() { let returns = vec![0.01, -0.01, 0.02]; let sortino = RiskServiceImpl::compute_sortino_ratio(&returns); - assert!((sortino - 0.0).abs() < 1e-10); + assert!( + sortino.is_none(), + "Should return None for insufficient data (not fake 0.0 — caller must distinguish \ + insufficient-samples from a legitimate zero-valued Sortino)" + ); } #[test] fn test_sortino_no_downside() { - // All returns well above risk-free => no downside deviation => 0 + // All returns well above risk-free => zero downside observations => + // downside deviation is undefined, MUST return None (not fake 0.0). let daily_rf = RISK_FREE_RATE_ANNUAL / 252.0; let high_return = daily_rf + 0.01; // well above risk-free let returns = vec![high_return; 10]; let sortino = RiskServiceImpl::compute_sortino_ratio(&returns); - assert!((sortino - 0.0).abs() < 1e-10, "No downside should yield zero Sortino"); + assert!( + sortino.is_none(), + "No downside observations must yield None (downside deviation is undefined), \ + not fake 0.0" + ); } #[test] @@ -1593,8 +1662,9 @@ mod tests { // Mix of positive and negative returns let returns = vec![-0.02, 0.03, -0.01, 0.02, -0.015, 0.01, 0.005]; let sortino = RiskServiceImpl::compute_sortino_ratio(&returns); - // Just verify it produces a finite number (not NaN/Inf) - assert!(sortino.is_finite(), "Sortino should be finite; got {}", sortino); + // Just verify it produces Some(finite) (not None/NaN/Inf) + let value = sortino.expect("Sortino should be Some(_) with mixed returns"); + assert!(value.is_finite(), "Sortino should be finite; got {}", value); } // -----------------------------------------------------------------------