fix: kelly_criterion missing avg-loss scaling, rf routed via inspect.stack() - #542
Open
aleks-drozy wants to merge 1 commit into
Open
Conversation
…stack() kelly_criterion() computed ((win_loss_ratio * win_prob) - lose_prob) / win_loss_ratio, which simplifies to a function of the win/loss *ratio* only, making it scale-invariant and off by a factor of |avg_loss()| versus the textbook growth-optimal Kelly fraction f* = win_prob/|avg_loss| - lose_prob/avg_win. Fix divides the existing result by abs(avg_loss(returns)), guarded the same way win_loss_ratio already is. Separately, _prepare_returns() decided whether to apply `rf` by inspecting inspect.stack()[1][3] (the caller's function name) against a hardcoded list, and only excluded "cagr" unconditionally regardless of what rf the caller passed - so cagr(r, rf=0.5) silently equalled cagr(r, rf=0.0). `if rf > 0` also silently dropped negative rf. Replaced the stack inspection with an explicit apply_rf parameter set at each call site (_prepare_benchmark, gain_to_pain_ratio, rolling_volatility keep rf unapplied as before; cagr now applies it, matching its documented "excess returns" behavior and how reports.py already calls it), and changed the guard to `rf != 0`. The cache key now includes apply_rf to avoid caching collisions between callers with the same (data, rf, nperiods) but different intent. Adds test_cagr_with_rf and test_kelly_criterion to TestRatios; both fail on the pre-fix code and pass after. Fixes ranaroussi#537
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #537
Root cause
1.
kelly_criterion()was off by a factor of the average-loss magnitude.This simplifies to
win_prob - lose_prob / win_loss_ratio, a function of the win/loss ratio only, so it is scale-invariant by construction. The textbook growth-optimal Kelly fraction for a two-outcome return series isf* = win_prob/|avg_loss| - lose_prob/avg_win, which factors to(win_prob - lose_prob/win_loss_ratio) / |avg_loss|— the code was missing the final division by|avg_loss()|.2.
rfwas routed by inspecting the call stack, andcagrwas unconditionally excluded regardless of therfthe caller passed._prepare_returns()usedfunction = inspect.stack()[1][3]to get the caller's function name and hardcoded"cagr"into aunnecessary_function_callsskip-list, sorfwas silently ignored whenever_prepare_returnswas called fromcagr— even thoughcagrtakes anrfparameter and is documented as computing CAGR "of excess returns", andreports.pyalready callscagr(df, rf, ...)expectingrfto matter.cagr(r, rf=0.0)andcagr(r, rf=0.5)returned identical values. Separately,if rf > 0silently dropped any negativerf.Fix
kelly_criterion: divide the existing (ratio-only) result byabs(avg_loss(returns))in both the scalar and DataFrame branches, with the same zero/NaN guarding already used forwin_loss_ratio._prepare_returns: replace theinspect.stack()-based dispatch with an explicitapply_rf: bool = Trueparameter. Call sites that previously relied on being named"_prepare_benchmark","gain_to_pain_ratio", or"rolling_volatility"now passapply_rf=Falseexplicitly, preserving their existing (rf-not-applied) behavior.cagr's call site is left at the default (apply_rf=True), sorfis now actually subtracted to compute excess returns, matching its docstring and howreports.pyalready calls it. Changed the guard fromif rf > 0toif apply_rf and rf != 0so negativerfis honored too. The_prepare_returnsresult cache key now also includesapply_rf, since two calls with identical(data, rf, nperiods)can legitimately want different treatment depending on the caller — this was a latent cache-poisoning bug in the old mechanism as well.Testing
Added to
tests/test_stats.py::TestRatios:test_kelly_criterion: deterministic two-outcome fixture (60 wins @ +2%, 40 losses @ -1%) with a closed-form expected value of40.0, plus a scale-dependence check (kelly(r*0.5) == 2*kelly(r),kelly(r*0.1) == 10*kelly(r)).test_cagr_with_rf: assertscagr(r, rf=0.02) != cagr(r, rf=0.0)and that a positiverfreduces the excess-return CAGR.Both tests were confirmed to fail against the pre-fix code (verified by stashing the fix and re-running) and pass after.
Ran the directly-touched test slice locally:
tests/test_stats.py+tests/test_utils.py: 49 passed, 0 failed (both before-fix baseline minus the 2 new tests, and after-fix full run).reports.metrics(returns, benchmark=..., rf=0.03, mode='full'),cagrwithrfof0,0.03, and-0.01(all now differ correctly),gain_to_pain_ratio, androlling_volatility— all compute without error and match prior (unapplied-rf) behavior where intended.Note:
tests/test_reports.pyshows pre-existing flakiness in this sandboxed Windows environment unrelated to this change — different tests intermittently fail/hang with a Tk/GUI backend error ("This probably means that tk wasn't installed properly") both with and without this fix applied (confirmed by re-running against the unmodified code). The specific test in isolation passes reliably.