Fix two critical bugs: avg entry price and equity curve VWAP benchmark#33
Merged
Fix two critical bugs: avg entry price and equity curve VWAP benchmark#33
Conversation
Critical:
- get_position() was computing avg_entry_price by blending buy and sell
costs into a single total_cost, then dividing by net_mw. After a partial
close at a different price (e.g. BUY 100@50 then SELL 50@60) the formula
returned 60 instead of 50 for the remaining long. Fix: track buy and sell
cost/volume separately; use total_buy_cost/total_buy_vol for net long
positions and total_sell_cost/total_sell_vol for net short. Three new
regression tests added.
- IDC equity curve snapshots used Decimal("0") as the VWAP benchmark
fallback when no market trades had occurred yet for a product, causing
fill PnL to be benchmarked against zero price and corrupting cumulative_pnl
(and therefore Sharpe, drawdown, and total_equity). Fix: fall back to
f.price (zero alpha — neutral until real VWAP data accumulates).
Cleanup:
- Removed the duplicate idc_vwap_accum accumulator from _run_idc. The
matching engine already accumulates the same data in _vwap_accum (added
in the previous fix for get_vwap). Exposed get_vwap_accumulator() on
ContinuousMatchingEngine so the engine loop can read it directly, removing
the dual-write maintenance hazard. Removed now-unused HistoricalTrade
import from backtest.py.
https://claude.ai/code/session_01XqYtc2t1oCV8eTmXN8aaiY
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
2 participants
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
get_position()returned wrongavg_entry_pricefor mixed buy/sell fills, causing incorrectunrealised_pnland corrupting Sharpe, drawdown, and total equity metrics downstream.cumulative_pnlin early MTUs._run_idc— the matching engine already tracks this internally since the previous fix; exposedget_vwap_accumulator()to avoid the dual-write.Details
Critical:
get_position()avg entry price (backtest.py:492)The old formula blended buy and sell costs into a single
total_costdivided bynet_mw. AfterBUY 100@50thenSELL 50@60, the remaining 50 MW long reportedavg_entry_price = 60instead of50. Root cause:abs(total_cost / net_mw)— the sell at 60 lifted the blended cost.Fix: track buy and sell cost/volume separately. For net long positions use
total_buy_cost / total_buy_vol; for net short usetotal_sell_cost / total_sell_vol. Three regression tests added covering pure long, partial-close long, and partial-buyback short.Critical: Equity curve zero VWAP fallback (
backtest.py:1174)running_vwaps.get(product_id, Decimal("0"))fell back to0when no historical trades had been seen yet for a product.compute_fill_pnlwith a zero benchmark makes every buy appear as a loss equal tofill_price × volumeand every sell a gain of the same magnitude — corruptingcumulative_pnland every metric built on it.Fix: fall back to
f.price(zero alpha — fill benchmarked against itself until real market VWAP data accumulates).Cleanup: Duplicate VWAP accumulation
Removed the
idc_vwap_accumlocal dict from_run_idcand exposedContinuousMatchingEngine.get_vwap_accumulator()instead. Both accumulated identical data; the dual-write was a maintenance hazard. Removed now-unusedHistoricalTradeimport frombacktest.py.Test plan
tests/test_delivery_position.py— three new regression tests for mixed buy/sell avg pricemake cipasses (727 tests, 0 failures)https://claude.ai/code/session_01XqYtc2t1oCV8eTmXN8aaiY