Repository navigation
feat: log realized volatility as a fill feature - #162
Conversation
Adds realized_vol_bps to the per-fill feature record (schema v1->v2) so the adverse-selection dataset includes the volatility regime, which prior offline analysis identified as the dominant driver of post-fill adverse selection (order-book microstructure features alone had near-random predictive power). - fill_features.compute_fill_features gains an optional realized_vol_bps arg (same shared definition for logging and future inference; no train/serve skew) - AdverseSelectionTracker.set_coin_volatility publishes per-coin RV; read at fill time into the record (GIL-atomic dict, no lock) - MarketMakingStrategy publishes its existing _compute_realized_volatility each cycle and opens the mid-recording gate when feature logging is on (so RV is available even when vol-adjust / dynamic-age are disabled) - bot.py links the tracker + enables publishing when fill-feature logging is on Observation-only; reuses the existing RV computation (no new deps/params/algo). Default behaviour unchanged when fill-feature logging is disabled.
keitaj
left a comment
There was a problem hiding this comment.
Review: log realized volatility as a fill feature
Clean, well-scoped, observation-only change that adds the realized-volatility regime to the fill-feature dataset. Core logic (feature passthrough, tracker publish/read, strategy gate + publish) is well covered by tests; default behaviour is preserved when fill-feature logging is disabled. No blockers; a few nits below.
Correctness / thread-safety (OK): _coin_vol is written by the strategy (main loop, _compute_ideal_prices) and read by the tracker (WS thread, _build_feature_record). Single-key dict get/set is GIL-atomic, matching the existing cross-thread pattern used by _fill_count / _aggregates, so no lock is required. RV reuses the existing _compute_realized_volatility (single source of truth) — no train/serve skew.
Backward compatibility (OK): schema bump v1 -> v2 is purely additive; realized_vol_bps is None until published. Consumers of v1 rows are unaffected.
1. Realized-vol unit/window not fully explicit for downstream modellers (nit)
fill_features.py, ws/adverse_selection_tracker.py:
realized_vol_bps is the mean absolute per-cycle mid return in bps over vol_lookback cycles (~a few minutes at the default loop interval), not an annualized or per-minute vol. This is correct and self-consistent (same definition serves training and future inference), but a future modeller could misread the unit.
Suggestion: note the exact definition/window in the schema doc (or a one-line comment where the field is emitted) so the unit is unambiguous.
2. RV publishing is coupled to BBO mode (nit)
strategies/market_making_strategy.py:
The publish call sits next to _record_mid_price inside the bbo_mode and bid>0 and ask>0 block, so realized_vol_bps is only populated in BBO mode. This mirrors where the mid buffer is already updated, so it is consistent — just noting that non-BBO configs would log null RV.
3. bot.py positive wiring path is untested (nit)
bot.py:
The hasattr(strategy, "_publish_vol_for_logging") branch that links the tracker and enables publishing is not directly covered (only the tracker-absent warning path is tested). This mirrors the existing, similarly-untested dynamic_offset wiring at the same site, and the positive path needs full WS init mocking, so severity is low.
Suggestion: optional follow-up — extend test_fill_feature_wiring.py if the WS-init path becomes easier to drive.
Verdict: LGTM
0 blockers, 0 should-fix, 3 nits. Observation-only, risk guards untouched, backward-compatible, and the core paths are well-tested. Proceeding to CI.
Summary
realized_vol_bpsto the per-fill feature record (feature schemav1→v2) so the adverse-selection dataset captures the volatility regime.Changes
fill_features.py:compute_fill_featuresgains an optionalrealized_vol_bpsargument and emits it;FEATURE_SCHEMA_VERSIONbumped to 2. Single shared definition keeps training and future inference in sync (no train/serve skew).ws/adverse_selection_tracker.py:set_coin_volatility(coin, vol_bps)publishes the latest per-coin RV; read at fill time into the record (single float per coin, GIL-atomic — no lock).strategies/market_making_strategy.py: publishes_compute_realized_volatility(coin)to the tracker each cycle, and opens the mid-recording gate when feature logging is enabled so RV is available even with vol-adjust / dynamic-age off. Guarded so quoting is unaffected.bot.py: links the tracker and enables RV publishing when fill-feature logging is on (guarded for non-MM strategies).README.md: notes realized volatility in the fill-feature description.compute_fill_features, tracker record inclusion (published / unpublished coin), and strategy-side gate + publish behaviour (incl. backward-compat when disabled or tracker absent).Test plan
flake8passespytestpasses (full suite)🤖 Generated with Claude Code