Skip to content

perf(simulation): prune Closed positions from simulator positions Map#1024

Merged
dayfine merged 1 commit into
mainfrom
perf/sim-positions-map-prune
May 10, 2026
Merged

perf(simulation): prune Closed positions from simulator positions Map#1024
dayfine merged 1 commit into
mainfrom
perf/sim-positions-map-prune

Conversation

@dayfine
Copy link
Copy Markdown
Owner

@dayfine dayfine commented May 10, 2026

Summary

  • The simulator's `t.positions : Position.t String.Map.t` retained every position indefinitely — every transition to `Closed` kept the entry via `Map.set`. Over a 15y run the Map grew to ~5000+ entries.
  • Per-trade `_find_position_by_symbol_state` and per-Friday `held_symbols` / `_initial_short_notional` all walked the full Map, paying O(closed + active) per call plus `Map.to_alist` / `Map.data` allocation pressure.
  • Fix: introduce `_set_or_drop_if_closed` helper at the two call sites (`_update_positions_from_trades`, `_apply_trigger_exit`). When `Trading_strategy.Position.apply_transition` / `_apply_fill` returns `Closed`, drop the entry instead of overwriting it.

Why this is observable-invariant

Every consumer of `portfolio.positions` already filters Closed:

  • `held_symbols` filters `Closed _ -> None` explicitly.
  • `_initial_short_notional` matches only `(Short, Holding _)`.
  • `portfolio_view.portfolio_value` contributes 0 for non-`Holding`.
  • `_find_position_by_symbol_state` requires `Entering` or `Exiting` state.

Audit trails live in independent sources (`Trade_audit`, `Stop_log`, `final_portfolio.positions` which already prunes closed). No consumer reads Closed positions from `t.positions`.

Perf measurement

10y Cell E h=2 on sp500-historical (510 syms) — same scenario, same simulator state:

Build Wall Trades Return Sharpe MaxDD
Pre-prune (main) killed at 17+ min still going
Post-prune 437s (7.3 min) 1155 +157.6% 0.83 15.2%

Pre-prune was ≥2.3× slower at the kill point and still climbing (consistent with O(closed×Fridays) creep — at year 8 of 10 most of the cost is iterating thousands of closed entries). Speedup grows with run length.

Earlier reference: 2026-05-09 `dev/notes/cell-e-15y-engineering-blocker-2026-05-09.md` recorded 15y Cell E "2h45m for 54% of cycles (476/882)" — projected ~5h+. Today's rolling-5y completed all 7 windows in 85 min total (~12 min each), and the post-prune 10y completes in 7.3 min, confirming the residual O(N) per-cycle hotspot is killed.

A 15y post-prune run is in flight to nail the final number.

Test plan

  • `dune build` clean
  • `dune runtest trading/simulation/test/` — all green (existing position-lifecycle tests cover the entry/exit transitions that hit `_set_or_drop_if_closed`)
  • 10y Cell E h=2 produces the same return / trade-count / Sharpe distribution as pre-fix (within numerical noise) — the metrics quoted above match prior 10y-window output
  • Add explicit unit test pinning "Closed positions absent from `t.positions` after exit fill completes" — followup; out of scope for the perf fix itself

🤖 Generated with Claude Code

@dayfine dayfine force-pushed the perf/sim-positions-map-prune branch from f4be20c to 35910c5 Compare May 10, 2026 22:09
@dayfine
Copy link
Copy Markdown
Owner Author

dayfine commented May 10, 2026

15y wall-clock result (post-prune)

Full 15+yr Cell E h=2 on sp500-historical (510 symbols, 2010-01-01..2026-04-30):

Value
Wall 818s (13.6 min)
Trades 2075
Return +235.3%
Sharpe 0.70
MaxDD 17.8%
WinRate 36.2%

Reference: 2026-05-09 dev/notes/cell-e-15y-engineering-blocker-2026-05-09.md recorded the pre-fix 15y run as "2h45m for 54% of cycles", projecting ~5+ hours for full completion. The post-fix 15+yr run completes in ~14 minutes — roughly 20-22× faster.

Sanity check on the O(N²)→O(N) claim: 10y/15y wall ratio is 437s/818s = 1.87×, which tracks the 10y/15y trade-count ratio of 1155/2075 = 1.80×. Per-trade cost is now constant, not growing with cumulative closed-position count.

Also pushed a follow-up commit fixing the ocamlformat docstring placement called out by qc-structural (280). Re-running QC.

@dayfine dayfine merged commit 356af45 into main May 10, 2026
2 checks passed
dayfine added a commit that referenced this pull request May 10, 2026
)

## Summary

Pre-existing latent bug surfaced by qc-behavioral during PR #1024
review. Two strategy helpers walk `portfolio.positions` to find or
detect a position for a given symbol but **don't filter by state** — so
a `Closed` entry for the same symbol matches as if it were active:

-
`trading/trading/strategy/lib/ema_strategy.ml::_find_position_for_symbol`
— would return a `Closed` position; downstream `_check_exit` then tries
to read a `Holding`/`Exiting` state and falls through silently.
-
`trading/trading/strategy/lib/bah_benchmark_strategy.ml::_has_position_for_symbol`
— would say "already entered" for a symbol whose prior position is
`Closed`, blocking a fresh re-entry.

Neither strategy is on the live path today (Cell E uses
`weinstein_strategy_screening.held_symbols` which already filters Closed
correctly), and PR #1024 prunes Closed positions from the simulator's
`t.positions` Map so the bug isn't observable in current backtests. But
this is a defensive belt-and-suspenders fix that mirrors the Weinstein
pattern and removes the dependency on simulator-internal behaviour.

## Test plan

- [x] `dune build` clean
- [x] `dune runtest trading/strategy/ trading/simulation/` — all green,
including `test_bah_benchmark_strategy`, `test_ema_strategy`,
`test_bah_benchmark_e2e`.
- No new test added — both helpers are private and never see Closed
positions today (simulator prunes them at the source per PR #1024). A
targeted unit test would exercise the post-prune Map invariant, which is
already covered by `test_full_position_lifecycle` in
`test_simulator.ml`.

## Authority

qc-behavioral on PR #1024
(`dev/reviews/pr-1024-simulator-positions-map-prune-behavioral.md`)
flagged these as pre-existing latent issues, scoped out of that PR.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@dayfine dayfine deleted the perf/sim-positions-map-prune branch May 11, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant