Bug: Drawdown metrics do not match portfolio snapshot data

[MDUYN]

Summary

The backtesting framework reports drawdown metrics that are inconsistent with the total_value series in the portfolio_snapshots it produces. Independently computing max drawdown from the snapshot total_value field yields significantly different results.

Expected values (from framework)

{
  "max_drawdown": 0.1384753616852372,
  "max_drawdown_absolute": 406.69151474500177,
  "max_daily_drawdown": 0.1384753616852372,
  "max_drawdown_duration": 240
}

Actual values (computed from portfolio_snapshots[].total_value)

Metric	Framework reports	Computed from snapshots	Delta
max_drawdown	13.85%	9.19%	+4.66pp
max_drawdown_absolute	406.69	230.50	+176.19
max_daily_drawdown	13.85%	7.84%	+6.01pp
max_drawdown_duration	240 days	241 days	-1 day

Analysis

1. Framework uses a different equity curve than total_value

The framework's absolute drawdown (406.69) divided by its fractional drawdown (0.1385) implies a peak equity of ~2,937. However, the actual peak total_value in the snapshots is ~2,508. This means the framework is computing drawdown from a different equity series than the one stored in total_value.

Possible causes:

• The framework may be summing fields differently (e.g. unallocated + pending_value + unrealized instead of using the pre-computed total_value).
• The framework may be revaluing positions at current market prices independently of the snapshot, producing a different equity curve.
• There may be a mismatch between the equity curve used internally for metrics and the one serialized to portfolio_snapshots.

2. max_daily_drawdown equals max_drawdown — likely a bug

The framework reports max_daily_drawdown = 0.1384753616852372, which is identical to max_drawdown. This is almost certainly wrong:

• max_daily_drawdown should represent the largest single-period (day-to-day) decline, which is typically much smaller than the peak-to-trough drawdown.
• From the snapshot data, the largest single-day drop is 7.84%, not 13.85%.
• If max_daily_drawdown truly equals max_drawdown, it would mean the entire 13.85% drawdown happened in a single snapshot interval — contradicting the reported max_drawdown_duration of 240 days.

Likely cause: max_daily_drawdown is being assigned the same value as max_drawdown instead of being computed independently as the worst single-period return.

3. max_drawdown_duration is close but off by 1 day

The duration (240 vs 241 days) is within rounding tolerance and may be an off-by-one in how the framework counts the start/end day (inclusive vs exclusive). This is minor.

How to reproduce

Using backtest_run_three.json:

import json
from datetime import datetime

with open("backtest_run_three.json") as f:
    data = json.load(f)

snaps = sorted(data["portfolio_snapshots"], key=lambda s: s["created_at"])

# Max drawdown from total_value
peak = 0
max_dd = 0
max_dd_abs = 0
for s in snaps:
    tv = s["total_value"]
    if tv > peak:
        peak = tv
    if peak > 0:
        dd = (peak - tv) / peak
        if dd > max_dd:
            max_dd = dd
            max_dd_abs = peak - tv

print(f"max_drawdown:          {max_dd}")       # 0.0919 — NOT 0.1385
print(f"max_drawdown_absolute: {max_dd_abs}")    # 230.50 — NOT 406.69

Suggested fix

1. Verify that the equity curve used for drawdown calculation matches the total_value written to portfolio_snapshots. If they diverge, either fix the metric calculation or fix the snapshot serialization.
2. Fix max_daily_drawdown to compute the worst single-period return independently:

   max_daily_dd = max(
       (snaps[i-1]["total_value"] - snaps[i]["total_value"]) / snaps[i-1]["total_value"]
       for i in range(1, len(snaps))
       if snaps[i-1]["total_value"] > 0
   )

3. Review the off-by-one in max_drawdown_duration (inclusive vs exclusive day counting).

[mac-agent]

Great bug report. I dug into the code and found the likely culprit.

Root cause

The drawdown functions in services/metrics/drawdown.py build an equity curve from portfolio_snapshots, but they skip zero or negative values with a continue:

for equity, _ in equity_curve:
    if equity <= 0:
        continue

This causes a peak tracking discontinuity. If the equity curve has any zero/negative values in the middle (which can happen during backtest initialization or when positions have negative mark-to-market), the running peak is not updated during those skipped iterations but IS updated afterward — effectively connecting a pre-gap peak to a post-gap value that should never be compared directly.

Illustration:

• Snapshot 0: total_value = 5000 → peak = 5000
• Snapshot 5: total_value = 0 (skipped)
• Snapshot 6: total_value = 2500 → compared against peak=5000 → drawdown = -50%
• But the actual equity only went 5000 → 0 → 2500, not 5000 → 2500 in one hop

Additional issue

get_max_drawdown_duration (line 137) increments current_duration by 1 per snapshot, not per day. If the framework takes snapshots every minute, a 240-pixel duration (matching the reported 240) represents minutes not days. This explains why max_drawdown_duration: 240 seems reasonable only if the sampling frequency is known.

Suggested fix

1. In get_max_drawdown / get_max_drawdown_absolute / get_max_drawdown_duration — when we encounter a skipped value, reset the tracking peak to None so the next positive value becomes the new peak.
2. In get_max_drawdown_duration — track actual time deltas (snapshot[i].created_at - snapshot[i-1].created_at) instead of incrementing by 1.

Happy to put together a PR for either or both of these if useful.

[MDUYN]

@mac-agent , Thanks for the thorough analysis! Both issues you identified were real bugs and have since been fixed in the current codebase.

1. Zero/negative value peak discontinuity

The continue-without-reset pattern you described has been replaced. The drawdown functions now handle zero/negative values properly:

get_max_drawdown finds the first positive value before starting peak tracking, then skips non-positive values without comparing them against a stale peak
get_drawdown_series resets the running peak when it detects max_value <= 0, so the next positive value becomes the new baseline
get_max_drawdown_absolute doesn't skip at all — it tracks peak - equity directly

2. Duration counting snapshots vs. days

get_max_drawdown_duration now uses datetime subtraction ((timestamp - drawdown_start).days) instead of incrementing a counter. This produces correct calendar-day durations regardless of snapshot frequency.

Both fixes are covered by 18 dedicated tests, including cases that specifically verify calendar-day counting with weekly snapshots and correct peak-reset behavior.

Appreciate the detailed write-up, your analysis matched exactly what we found and fixed.