fix: direction-aware pnl_percent and trade cost decomposition - #5
Merged
Merged
Conversation
Short P&L fix: pnl_percent now returns positive for profitable shorts and negative for losing shorts, matching long-side convention. Fixed in Position.pnl_percent(), FillExecutor close/flip, and Engine open-trade results. Trade cost decomposition: new fields entry_slippage and multiplier on Position/Trade, plus computed properties gross_pnl, net_pnl, net_return, total_slippage_cost, and cost_drag. Parquet schema updated with backward compatibility for old files. 37 new tests in test_trade_cost_decomposition.py.
There was a problem hiding this comment.
Pull request overview
This PR updates backtest trade/position accounting to make pnl_percent direction-aware (profitable shorts yield positive returns) and adds trade cost decomposition fields/derived metrics (including entry slippage and futures multipliers), with Parquet backward compatibility.
Changes:
- Fix direction-aware
pnl_percentfor shorts acrossPosition, position close/flip execution, and open trade mark-to-market results. - Add
entry_slippageandmultiplierfields plus computed properties (gross_pnl,net_return,total_slippage_cost,cost_drag) for trade cost decomposition. - Extend Parquet schema + loader defaults for backward compatibility, and add extensive new tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_trade_cost_decomposition.py |
Adds comprehensive tests for short P&L sign correctness, MFE/MAE behavior, cost decomposition properties, futures multipliers, and Parquet roundtrips/back-compat. |
tests/test_result.py |
Updates expected trades DataFrame columns to include entry_slippage and multiplier. |
src/ml4t/backtest/types.py |
Adds Position.entry_slippage, Trade.entry_slippage/multiplier, and new computed cost decomposition properties; updates Position.pnl_percent() to be direction-aware. |
src/ml4t/backtest/result.py |
Writes/reads new Parquet columns (entry_slippage, multiplier) and updates trades schema. |
src/ml4t/backtest/execution/fill_executor.py |
Propagates entry slippage into positions/trades and fixes close/flip pnl_percent sign handling. |
src/ml4t/backtest/engine.py |
Fixes open-trade mark-to-market PnL to include multiplier and applies direction-aware pnl_percent. |
src/ml4t/backtest/analytics/trades.py |
Updates total_slippage aggregation to use dollar cost via total_slippage_cost. |
src/ml4t/backtest/analytics/bridge.py |
Exposes new fields and computed decomposition metrics in diagnostic trade records. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add comprehensive test infrastructure: - Autouse accounting invariants (conftest.py) checked on every Engine.run() - Exit-reason consistency checks (stop_loss→loss, take_profit→gain, etc.) - Fill-level order-type bound checks (limit buy never overpays, etc.) - Fill metadata enrichment: 3 optional fields on Fill dataclass - Direction x order-type matrix (17 tests: limit/stop fills, gap-through, PnL) - Bracket order lifecycle (16 tests: TP/SL cancel, gap-through, sequential) - Property-based order-type invariants (5 Hypothesis tests, 900 examples) - Independent oracle engine for differential testing - Parametric scenario factory - Direction matrix tests (long/short x multiple strategies) - PnL property-based invariants 1367 passed, 14 skipped. All quality gates clean.
…, docs audit - TradeAnalyzer: avg_win, avg_loss, largest_win, largest_loss, expectancy, avg_trade now all percentage-based (select from pnl_percent, not dollar PnL) - Added payoff_ratio (avg_win / |avg_loss|) - result.metrics includes cost decomposition: total_gross_pnl, total_costs, avg_cost_drag, gross_profit_factor - to_trades_dataframe() includes 4 computed columns: gross_pnl, net_return, total_slippage_cost, cost_drag - Rewrote docs/user-guide/results.md with Trade Analyzer section, cost decomposition section, Fill metadata section, percentage-based examples - Fixed LIMITATIONS.md: Mode.REALISTIC -> BacktestConfig.from_preset() - Audited and corrected all 10 AGENT.md files: fixed line counts, removed fictional class names (CashAccountPolicy, SignalStrategy etc.), updated to actual classes (UnifiedAccountPolicy, SignalFollowingStrategy etc.)
- Scale-in cost accumulation: entry_commission and entry_slippage now accumulated on position scale-up (was losing intermediate leg costs) - Rebalancer sell-before-buy: INCREMENTAL/HYBRID modes now process reduces before increases, preventing cash-starved buy rejections - Session alignment in to_portfolio_analysis(): auto-detects CME calendars and passes session_aligned=True to to_daily_pnl() - MAE recovery sign: (pnl_percent - mae) / |mae| gives positive recovery ratio as documented - Zero close price: use `is not None` instead of truthy check in datafeed and rebalancer price guards - Minute break gaps: generate_trading_minutes() splits around break_start/break_end for exchanges like XTKS - Intraday annualization: EquityCurve.years uses wall-clock time, new periods_per_year property, sharpe/sortino/volatility properties use inferred bar frequency - Tearsheet slippage: fallback uses total_slippage_cost (dollar cost) instead of per-unit exit slippage All 8 fixes include regression tests. 1375 passed, 14 skipped.
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.
Summary
pnl_percentnow direction-aware — positive for profitable trades, negative for losing trades, regardless of long/short. Fixed inPosition.pnl_percent(),FillExecutorclose/flip, andEngineopen-trade results.entry_slippageandmultiplieronPosition/Trade, plus computed propertiesgross_pnl,net_pnl,net_return,total_slippage_cost, andcost_drag.entry_slippage=0.0,multiplier=1.0defaults).Test plan
test_trade_cost_decomposition.py