feat: FeedSpec carries vwap_col so a feed can declare a volume-weighted price - #13
Merged
Conversation
…ed price ExecutionPrice.VWAP has existed in ml4t-backtest since the enum was written and has never had a column to read: there is no vwap_col anywhere in the feed contract, so the broker resolved it to the close and, under next_bar execution, to the open. Neither substitution was reported. vwap_col defaults to None rather than to a guessed column name. Open, high, low, close and volume all have conventional names worth defaulting to; VWAP does not, and a consumer asking for it is asking for a price no other column approximates. The close is not a worse VWAP, it is a different quantity, so a feed that does not carry one must say so and let the consumer refuse. Refs ml4t/agent-workspace#1017.
There was a problem hiding this comment.
🟡 Changes recommended
vwap_col is added to FeedSpec but not to MarketDataSchema, so VWAP cannot be declared/preserved through the common MarketDataSpec -> FeedSpec projection path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the FeedSpec market-data contract to explicitly carry an optional vwap_col so downstream consumers can request VWAP without silently substituting another price column.
Changes:
- Add
vwap_col: str | None = NonetoFeedSpecand include it in_FIELD_ALIASESso it round-trips throughfrom_mapping/from_object/from_any. - Add unit tests asserting the default (
None) and basic construction/override behaviors forvwap_col.
File summaries
| File | Description |
|---|---|
src/ml4t/specs/market_data.py |
Adds vwap_col to FeedSpec and alias extraction so feeds can declare a VWAP column explicitly. |
tests/test_market_data.py |
Adds test cases validating vwap_col defaults, mapping/object round-trips, and with_overrides behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
stefan-jansen
added
compatibility: affected
Merged
stefan-jansen
added a commit
to ml4t/backtest
that referenced
this pull request
Sep 3, 2026
FeedSpec.vwap_col shipped in ml4t-specs 0.1.4 (ml4t/specs#13, b17105b), so the temporary [tool.uv.sources] git pin that let the `ty` hook resolve the field is no longer needed. Delete it and raise the floor to >=0.1.4,<0.2. The pin was also what made every Compatibility job red: those jobs run with --locked, and a git-sourced dependency cannot satisfy a lockfile check against the registry.
stefan-jansen
added a commit
to ml4t/backtest
that referenced
this pull request
Sep 3, 2026
* fix: ExecutionPrice.VWAP fills at the feed's VWAP, or refuses ExecutionPrice.VWAP has been declared since the enum was written and never read a VWAP. broker.py resolved it to `self._current_prices.get(asset, self._current_closes.get(asset))` - character-for-character what ExecutionPrice.PRICE returns - and there was no VWAP anywhere in the feed contract for it to read. The branch was also unreachable in practice. The use_open short-circuit excluded only BID, ASK, QUOTE_MID and QUOTE_SIDE, so under ExecutionMode.NEXT_BAR a VWAP request returned the next bar's *open* whatever that branch said. Fixing the lookup alone would have changed nothing observable while making every artifact a reviewer opens - the enum, the config, the data, the new store - read correctly. Both halves are here for that reason. - FeedSpec.vwap_col declares the column; DataFeed carries it into _AssetsData._vwaps and MarketState.vwaps beside the existing OHLCV caches. - ExecutionPrice.VWAP joins the use_open exclusion set: a VWAP is already a whole-bar price and the open must not override it. - A missing VWAP raises instead of falling back. A VWAP fill and a close fill are different assumptions, and substituting one for the other is indistinguishable from a correct run - which is how this survived since the enum was written. - _update_time keeps its existing 2- and 8-positional forms working and gains a 9-positional and a vwaps= keyword form. The compatibility snapshot is regenerated: both surface changes are additive - a nullable field and a keyword-only parameter with a default - so no existing caller breaks. Verified: 1966 passed. tests/test_vwap_execution_price.py uses an open, close and VWAP that are mutually distinct, and 6 of its 9 cases fail against the previous broker; a fixture where any two coincide could not. Depends on ml4t/specs#13, pinned here through a temporary [tool.uv.sources] git source that must be removed before merge. Closes ml4t/agent-workspace#1017. * fix: a bar with no VWAP is a non-fill, not a refusal Amends the previous commit, which conflated two different situations behind one raise. The nasdaq100_microstructure lane asked whether the raise fires upstream of the skip that implements a non-fill, and it does: the fill path is FillEngine.get_fill_price_for_order -> broker.get_price_for_source( broker.execution_price, ...), and every caller in execution_engine.py guards it with `if price is None: continue`. That `continue` is already the behaviour we want - the order does not fill, the prior position stands, no new machinery - and it needs a None. Raising there turned an ordinary no-trade bar into a stopped run. The two cases are now separated by where they are caught: - A feed that declares no vwap_col at all, under execution_price=VWAP, is a configuration error. Engine rejects it once, before the first bar. - An individual asset-bar with no VWAP is an ordinary market state: nothing traded, so there is no volume-weighted price. get_price_for_source returns None and the order goes unfilled. Neither falls back to the close, which is the original defect: on a no-trade bar the close is a stale carried print, so substituting it would invent a fill at a price nothing transacted at. Measured, because the rate decides whether this matters: on the NASDAQ-100 production universe (102 symbols, March 2021) 0.2384% of symbol-minutes inside 09:30-16:00 carry no print, worst name CTAS at 2.95%. Across all hours it is 5.02%, but that population is pre- and post-market and this case study trades the exchange session. Verified: 1970 passed. The refusal tests become non-fill tests, and three new cases cover the configuration guard: it raises on an undeclared column, accepts a declared one, and stays out of the way under any other execution price. * Depend on released ml4t-specs 0.1.4 instead of the branch pin FeedSpec.vwap_col shipped in ml4t-specs 0.1.4 (ml4t/specs#13, b17105b), so the temporary [tool.uv.sources] git pin that let the `ty` hook resolve the field is no longer needed. Delete it and raise the floor to >=0.1.4,<0.2. The pin was also what made every Compatibility job red: those jobs run with --locked, and a git-sourced dependency cannot satisfy a lockfile check against the registry. * test: update the ml4t-specs minimum version contract
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.
ExecutionPrice.VWAPhas existed inml4t-backtestsince the enum was written and has never had a column to read. There is novwap_colanywhere in the feed contract, so the broker resolved it to the close and, undernext_barexecution, to the open. Neither substitution was reported. Filed as ml4t/agent-workspace#1017; the broker fix is ml4t/backtest#TBD and depends on this.The field
It defaults to
Nonerather than to a guessed column name.open,high,low,closeandvolumeall have conventional names worth defaulting to. VWAP does not, and a consumer asking for it is asking for a price that no other column approximates - the close is not a worse VWAP, it is a different quantity. A feed that does not carry one must say so, and let the consumer refuse rather than substitute.Added to
_FIELD_ALIASESwith a single alias, so it round-trips throughfrom_mapping,from_objectandwith_overrideslike every other column.Verification
367 passed, coverage held at 100%. Five new cases cover the default, both construction paths,with_overrides, and that an override ofNonedoes not clear a column already declared.