Deprecate LinearImpact.permanent_fraction, which calculate() never read - #92
Deprecate LinearImpact.permanent_fraction, which calculate() never read#92stefan-jansen wants to merge 8 commits into
Conversation
The field was stored on the dataclass, documented as "Fraction of impact that is permanent (0-1). Remainder is temporary and reverts", and read nowhere: LinearImpact(coefficient=0.1, permanent_fraction=0.0) and the same model at 1.0 both return 1.0000000000000002 for a 10% participation order at $100. A reader who set 0.8 because they wanted a mostly permanent model got a model with no persistence at all and no warning. It cannot be honoured where it sat. `calculate` sees one order and has no reference to the ones before it, and `fill_executor` applies the returned impact to that order's fill price only - nothing is carried into the price for later orders. That is true of all four models here, so the parameter is removed rather than given state or a split return: LinearImpact is a single-order concession model and the docstring now says so, with persistence named as the caller's business. Two tests replace the one that asserted the default value: ten identical slices are charged the same concession by every model, and passing permanent_fraction now raises TypeError instead of being silently ignored.
The reviewed public-surface snapshot pins every exported signature, so removing LinearImpact.permanent_fraction moves one line in it.
The retained evidence pins `engine: _tree_digest(SOURCE_DIR)`, a SHA over every tracked .py under src/ml4t/backtest, so removing an unread dataclass field marks it stale even though nothing it measures can move. Rebuilt from the retained 2026-09-03 comparison outputs and the frozen input bundles. 17 of 17 required pairs pass and every record is byte-identical to the shipped file: the whole diff is four lines - `generated_at`, the ML4T commit, the engine digest, and the interpreter that built the report. An inert change producing an identical record set is the evidence that it is inert. The corpus builder was deliberately not run. `real_strategy_corpus.py` re-selects the workloads from the current registries rather than re-measuring the retained ones, so running it would silently rescope the audit. It also cannot run today: us_equities_panel has 31 training runs, 166 prediction sets and zero backtest rows, and selection is by validation backtest Sharpe, so it has no rank-1 candidate.
66 required pairs pass and 2 stay unsupported, exactly as before, across the four scenario-matrix frameworks in environments built inside this worktree so the matrix exercises this branch's engine and not the sibling checkout's - verified by importing ml4t.backtest in each of the four and checking the path. Ignoring the fields a re-run necessarily moves (timings, digests, runtime identity), every record is equal to the accepted evidence and `release_gate_passed` stays true.
The retained evidence pins the engine tree digest, so removing an unread field marks the timing file stale along with the rest. This re-measures all 17 pairs and regenerates the published tables from the result. The measurement is honest but less precise than the file it replaces, and the reason is the machine rather than the engine. The shipped evidence holds every one of its 34 timed engines under an 8.3% relative 95% CI; this run has five above 8%, worst 54%. Two earlier runs were rejected outright at 76% and 54% worst-case. All three were started on a quiet box and overrun: during this one the load median was 5.0, p90 12.2, with a peak of 108.5. One row is not merely wider. fx_pairs/lean reports an ML4T median of 0.485s (0.423-0.684) where the shipped file says 0.153s (0.152-0.157), moving that published ratio from 5.952x to 1.493x. Nine of its ten samples sit between 0.36 and 0.91 while the same engine measured 0.160s under the vectorbt_pro profile in the same run, and a run on the previous dependency set measured it at ~0.15s. It is contention, not a change in the engine, and it should be re-measured on an idle machine before anyone quotes that number. Removing the field changed no engine output. Every retained ML4T parquet was re-derived and checked against the shipped evidence: of 51 hashes across the 17 pairs, 26 are identical and the other 25 reproduce the shipped value byte for byte when the same frame is written by polars 1.36.1, the version this repository pinned before 0.1.6. Zero unexplained. The hashes moved because 0.1.6 bumped polars to 1.44.1, which encodes string- and null-heavy frames differently; equity.parquet on the smaller workloads encodes identically under both, which is why only some rows moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WfX5GbAHc5EVwdUnRSoUPT
|
Re-verified the defect and the blocker against The defect is live. The source fix in this PR is right and is not what is holding it up. Removal rather than What holds it up is the evidence binding, and it is structural. I tried to land the The same tests pass unmodified on And the available measurement is not quotable. That same commit establishes the change is inert: all 51 retained output hashes across the This machine cannot produce a better measurement today - load average 24, a 25 GB VM, a So the three ways forward are re-measure on a quiet machine, give the evidence tooling a way |
…permanent-fraction
…right The field is inert: `calculate()` has never read it, so `permanent_fraction=0.0` and `permanent_fraction=1.0` charge the same price. It cannot be honoured as the signature stands - `calculate` sees one order and holds no reference to the ones before it - so the model is a single-order concession model and persistence belongs to the caller. This branch removed the field. Removing it in a patch release raises `TypeError` on code that constructs `LinearImpact(permanent_fraction=...)` today and gets nothing for it, so the removal moves to 0.2.0 and this release warns instead. A reader who set the parameter currently gets silence; a `DeprecationWarning` naming the removal is strictly better, and it costs nobody a broken import. The warning lives in `__setattr__` rather than `__post_init__` because the field is settable after construction as well as through it, and a non-frozen dataclass routes `__init__` through `__setattr__`, so one guard covers both paths and fires once per assignment. The default is the one value that cannot warn: a dataclass cannot tell a caller who passed 0.5 from one who passed nothing, and leaving it alone is the case that loses nothing when the field goes. Tests pin three things, and the third is what makes the first two mean something: setting it warns and does not change the charge; assigning it after construction warns; and the untouched default raises no warning at all. Without the negative case a warning on every construction would pass. The retained `v0.1.json` compatibility snapshot goes back to main's line, because the signature is unchanged.
Any edit under `src/ml4t/backtest` moves `_tree_digest(SOURCE_DIR)`, and all three
retained evidence files record it. `test_parity_claim_generation.py` and
`test_real_strategy_runner.py` compare that recorded digest against the working tree, so
a source change of any size - this one is a docstring, a default and a `__setattr__` -
invalidates the correctness evidence, the real-strategy evidence and the timing evidence
together. Regenerating all three is the price of the fix, and the timing third of it
needs a quiet machine.
This branch regenerated them on 2026-09-09 and the timing third came out worse than the
file it replaced. Measured over the 34 timed engines in both, as relative width of the
95% CI about the median:
shipped (2026-09-03): worst 8.3% (fx_pairs/lean framework), 1 engine above 8%
regenerated (2026-09-09): worst 53.7% (fx_pairs/lean ML4T), 5 engines above 8%
Eight of the seventeen published ratios move by more than 10%, the largest being
fx_pairs/lean 5.952x -> 1.493x and us_equities_panel/backtrader 23.082x -> 33.987x. The
fx_pairs row is the one the re-measurement flagged itself: the same ML4T engine timed
0.160s under the vectorbt_pro profile in the same run, so 0.485s (0.423-0.684) is
contention rather than the engine. Those numbers are published in README.md, docs/index.md
and docs/user-guide/profiles.md, where a reader quotes them.
So the shipped 2026-09-03 table stands here and the three documents keep it. The branch is
red until all three files are regenerated together against this tree, which is a ~3 hour
exclusive measurement (17 pairs, one warm-up and ten samples a side, us_equities_panel/
backtrader alone at 549s a sample) and is not something to run beside two production case
study chains.
stefan-jansen
changed the title
|
This PR is blocked by the release-evidence gate, not by anything in the diff.
That is why all fifteen qualification jobs are red here while the diff itself is sound. Worth recording: because this PR reads as failing rather than as an open fix, a second branch ( The underlying problem is the granularity. Correctness evidence should re-derive on any source change - you cannot know behaviour held without checking. Timing evidence pinned to a whole-tree source hash means every docstring edit in the engine prices a 2h29m benchmark re-run, which is the cost that will keep recurring after this keyword is resolved either way. |
|
Superseded by #93, which is merged as 000139e. #93 removes #93 also fixes the reason both this PR and that one sat red: the evidence gate pinned a whole-tree source digest into the published timings, so any source edit priced a 2h29m benchmark re-run. Published timings now stay attributed to the source that produced them, and a later source that moves no measured value is certified against re-derived correctness evidence instead. Both evidence files were re-derived: 17/17 real-strategy pairs, 66 correctness scenarios, and of 231,740 leaf values in the correctness matrix only 201 moved, all of them durations, digests, commits or timestamps. Thanks for the deprecation groundwork - the test that the parameter is genuinely unread carried over in spirit as |
LinearImpactstores apermanent_fractionfield, documented as "Fraction of impact that ispermanent (0-1). Remainder is temporary and reverts", that
calculate()never reads.LinearImpact(coefficient=0.1, permanent_fraction=0.0)and the same model at1.0charge everyorder the same price. The engine has no state in which a permanent component could act:
fill_executorapplies an impact model to the order in front of it, so ten identical slices are tenidentical concessions no matter what the field says.
What this PR now does
The parameter is deprecated, not removed. Removing it in a patch release raises
TypeErroroncode that constructs
LinearImpact(permanent_fraction=...)today - code that already gets nothingfor it. So this release warns and 0.2.0 removes:
The warning lives in
__setattr__, not__post_init__, because the field is settable afterconstruction as well as through it and a non-frozen dataclass routes
__init__through__setattr__. One guard covers both paths. The default is the one value that cannot warn: adataclass cannot tell a caller who passed
0.5from one who passed nothing, and leaving it aloneis the case that loses nothing when the field goes.
Three tests, and the third is what makes the first two mean something: setting it warns and does
not change the charge; assigning it after construction warns; the untouched default raises no
warning at all. Without that negative case, a warning on every construction would pass. The
docstring says the model is a single-order concession model and that the field is inert, and the
retained
v0.1.jsoncompatibility snapshot is back at main's line because the signature isunchanged.
🔴 This branch is red, and merging it needs a quiet machine
validation/common/provenance.pypinsengine: _tree_digest(SOURCE_DIR), a SHA over every tracked.pyundersrc/ml4t/backtest, into all three retained evidence files.tests/contracts/test_parity_claim_generation.pyandtests/contracts/test_real_strategy_runner.pycompare that recorded digest against the working tree, so any source edit - this one is a
docstring, a default and a
__setattr__- invalidates the correctness evidence, the real-strategyevidence and the timing evidence together. Seven tests fail until all three are regenerated against
this tree. There is no version of this fix that avoids it.
The earlier revision of this branch regenerated them on 2026-09-09 and the timing third came out
materially worse than the file it replaced. Measured over the 34 timed engines in both files, as
relative width of the 95% CI about the median:
Eight of the seventeen published ratios moved by more than 10%, the largest being fx_pairs/lean
5.952x -> 1.493x and us_equities_panel/backtrader 23.082x -> 33.987x. The fx_pairs row is the one
that run flagged itself: the same ML4T engine timed 0.160s under the
vectorbt_proprofile in thesame run, so 0.485s (0.423-0.684) is contention rather than the engine. Those numbers are published
in
README.md,docs/index.mdanddocs/user-guide/profiles.md, where a reader quotes them, sothe shipped 2026-09-03 table stands in this branch and the regenerated files are reverted.
What it takes to go green. All three evidence files regenerated together against this tree:
The third is the expensive one and it is the one that needs the box to itself: 17 pairs, one warm-up
and ten measured processes a side, with
us_equities_panel/backtraderalone at ~549s a sample -roughly three hours during which nothing else may run. It was not run here because two production
case-study chains are live on this machine and the 2026-09-09 attempt already shows what a loaded
box does to the numbers. The corpus builder stays unrun either way:
real_strategy_corpus.pyre-selects workloads from the current registries rather than re-measuring the retained ones.
What the earlier revision established, and still holds
Removing the field changed no engine output, and that was shown independently of the timings: of 51
retained ML4T output hashes across the 17 pairs, 26 were identical and the other 25 reproduce the
shipped value byte for byte when the same frame is written by polars 1.36.1. Every
equity.parquetand
rejected_orders.parquethash was unchanged;fills.parquetmoved for every workload and thatis the polars 1.36.1 -> 1.44.1 bump that landed on main in 0.1.6, not a behaviour change - reading
the new file and writing it straight back under 1.36.1 with the runner's options reproduces the
shipped hash. A deprecation warning can move even less than a field removal could.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EKVuCYKyzMyRG6SEm8HwqU