feat: portfolio performance analytics (Sharpe, Sortino, Max DD, Win Rate)#1
feat: portfolio performance analytics (Sharpe, Sortino, Max DD, Win Rate)#1biswajeetdev wants to merge 2 commits into
Conversation
Adds broker/performance.py with Sharpe, Sortino, Max Drawdown, Win Rate, Profit Factor, and Avg Hold Time computed from trade_history.json and equity curve. Wires the report block into the daily email summary so every close-of-day email includes a structured performance snapshot.
biswajeetdev
left a comment
biswajeetdev
left a comment
There was a problem hiding this comment.
Code review pass — a few items to fix before this is production-ready.
| if len(equity) < 2: | ||
| return np.array([]) | ||
| arr = np.array(equity, dtype=float) | ||
| return np.diff(arr) / arr[:-1] |
There was a problem hiding this comment.
Zero-division risk. If any equity snapshot is 0.0 (e.g. before the first deposit), arr[:-1] contains a zero and np.diff(arr) / arr[:-1] silently produces inf/nan, which propagates through Sharpe/Sortino giving garbage ratios.
Fix: strip zero snapshots before computing returns:
arr = arr[arr != 0]
if len(arr) < 2:
return np.array([])
return np.diff(arr) / arr[:-1]| return np.diff(arr) / arr[:-1] | ||
|
|
||
|
|
||
| def sharpe_ratio(equity: list[float]) -> float | None: |
There was a problem hiding this comment.
Python 3.10+ syntax. float | None union shorthand requires Python 3.10+. Use Optional[float] from typing for 3.8/3.9 compatibility — applies to sharpe_ratio, sortino_ratio, and avg_hold_days.
| losses = [t["pnl"] for t in closed if t["pnl"] <= 0] | ||
|
|
||
| gross_profit = sum(wins) | ||
| gross_loss = abs(sum(losses)) or 1e-9 |
There was a problem hiding this comment.
Misleading profit factor on perfect win rate. abs(sum(losses)) or 1e-9 returns 1e-9 when there are no losing trades, making profit_factor astronomically large (e.g. 4.2e+11) in the Telegram/email output.
Fix:
gross_loss = abs(sum(losses))
profit_factor = round(gross_profit / gross_loss, 3) if gross_loss > 0 else NoneThe fmt() helper in format_for_summary already handles None gracefully.
| entry = datetime.fromisoformat(t["entry_date"]) | ||
| exit_ = datetime.fromisoformat(t.get("exit_date") or t.get("close_date") or "") | ||
| durations.append((exit_ - entry).days) | ||
| except Exception: |
There was a problem hiding this comment.
Silent data loss. except Exception: continue swallows KeyError on trade records missing entry_date, dropping corrupted entries with no visibility.
Add a log line at minimum:
except Exception as e:
print(f"[perf] skipping trade in avg_hold_days: {e}")
continue…tor, logging - Strip zero equity snapshots before computing daily returns to prevent inf/nan - Replace float|None union syntax with Optional[float] for Python 3.8/3.9 compat - Fix profit_factor: return None (not 1e-9 floor) when there are no losing trades - Log skipped trades in avg_hold_days instead of silently dropping them
|
Addressed all four review points in the follow-up commit (0b3f016):
|
1 participant
Summary
broker/performance.py— standalone analytics module that computes key performance metrics from local trade history and equity curveMetrics computed
Design notes
Nonefor metrics that need more data points (min 20 equity observations for ratio calculations) rather than crashingget_performance_report()is the single public entry point; returns a flat dict safe to log/serialiseformat_for_summary()produces a fixed-width text block for Telegram / emailpython -m broker.performanceTest plan
python -m broker.performanceprints N/A gracefully when trade_history.json is emptyNone(not crash) with fewer than 20 equity data points