Fix max_drawdown inventing drawdowns for price series - #541
Open
nyxst4ck wants to merge 1 commit into
Open
Conversation
_get_baseline_value guessed the pre-series equity from the price level (>1000 -> 1e5, >10 -> 100, else 1.0). When the guess landed above the first price it became a peak the portfolio never reached: an S&P-level series reported -95% instead of -7.7%, BTC-level -45% instead of -11%, and a DataFrame applied one column's baseline to every other column. Stop guessing. Returns rebuilt by _prepare_prices start from base 1.0, so use exactly 1.0; a series that was already prices starts its record at the first observation, so the phantom point takes that value and is a no-op. The day-one-loss fix from ranaroussi#438 (returns input) is preserved and pinned by tests.
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.
The bug (unreported)
qs.stats.max_drawdown()andto_drawdown_series()report drawdowns that never happened when given a real price series:[5000, 5100, 4900, 5200, 4800, 5050][50, 55, 52, 60, 54, 58]Root cause
_get_baseline_value()guesses the pre-series "starting equity" from the price level (>1000 → 1e5,>10 → 100, else1.0). The phantom baseline point prepended by the #438 fix (so a day-one loss isn't hidden) then takes that guessed value — and when the guess lands above the first price, it becomes a peak the portfolio never reached, and every drawdown is measured from that fiction. The bug fires whenever the first value falls in (10, 100) or (1000, 100000) — most equity index levels, crypto, and mid-priced stocks. Outside those bands the guess sits below the series and is absorbed by the expanding max, which is why it went unnoticed.The fix: stop guessing
_prepare_prices()rebuilds prices as1.0 × (1 + compsum), so the true baseline is exactly1.0— the max drawdown calculation is incorrect if the first return is a drawdown #438 day-one-loss behavior is preserved and now pinned by a test.New shared
_looks_like_returns()inutils.pyreplaces the two inline copies of the same predicate inside_prepare_prices()(each path's exact semantics preserved — Series usedmin() < 0, DataFrame columnsmin() <= 0), so the conversion and the baseline decision can't drift apart.One deliberate behavior change to be transparent about
qs.utils.to_prices(returns)output (default base 1e5) piped intomax_drawdownno longer counts a first-period loss measured from the 1e5 notional start — the base is unknowable from the series alone, and the alternative is continuing to corrupt genuine market price series.to_portfolio_value()already handles this honestly by prepending a real starting-balance row instead of guessing.Validation
TestDrawdownclass: 5 fail on main with the exact wrong values above, all green with the fix. The passing parametrizations (scale 1.0, 100.0) pin the boundary where the old guess happened to be harmless.Unrelated to #533 (weekly aggregation), which stays as a separate PR.