Remove the inert LinearImpact.permanent_fraction, and stop pricing a benchmark re-run for source edits that move no number - #93
Merged
Conversation
LinearImpact accepted and documented a permanent_fraction field that calculate() never referenced. LinearImpact(coefficient=0.1, permanent_fraction=0.0) and the same call with 1.0 both return 1.0000000000000002 on a 10% participation at $100, so a caller asking for a mostly permanent impact model got a fully temporary one and no warning. It cannot be honoured as written: calculate() is handed one order and holds no reference to earlier slices of the same parent, so there is nothing for a permanent component to persist into. Removing the field rather than defaulting it turns a silent wrong answer into a TypeError. Nothing in this repo, the book repo or the book read it, so no caller changes behaviour. The reviewed v0.1 surface snapshot moves with it, per the documented --write-and-review path in docs/user-guide/accounts.md. Also fixes two market-impact doc examples that passed eta= to LinearImpact and SquareRootImpact, whose field is coefficient=; both raised TypeError as written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPDj1nioTuD4xATMVyhZxB
real_strategy_benchmark._command already builds all 34 framework/ml4t invocations correctly, including the two lean special cases and the per-pair --comparison-profile and --price-decimals from PROFILES. But _measure_side runs each one inside a TemporaryDirectory because it wants timings, so no equity.parquet or fills.parquet ever survives and real_strategy_evidence.py has no tree to compare. This driver imports _command and _run_once, enumerates the 17 required pairs the same way build_report does, and runs each side once into a durable tree. It collects no timings and says so, so a one-sample run is never mistaken for a published measurement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPDj1nioTuD4xATMVyhZxB
…for it _tree_digest hashes every tracked .py under src/ml4t/backtest, so any source edit - a docstring included - invalidates REAL_STRATEGY_PERFORMANCE.json and prices a full re-measurement: 374 subprocesses, about 2h29m of engine time, and it has to run on an otherwise idle machine or the confidence intervals are noise. Removing a constructor parameter that no code path read was about to cost exactly that. ml4t_engine_source_sha256 now means what it says and is never rewritten: the source the timings were actually measured under. A later source that moves no measured value is recorded in certified_equivalent_sources instead, and the gate accepts either. The report therefore keeps saying which source produced its numbers while also saying which sources they still apply to. Certification is not a waiver. Each entry carries the correctness evidence it rests on, that evidence's own digest and generation time, the passing pair count, the commit, and a required written reason - because correctness parity does not prove a change is performance inert (an added loop can leave every number identical and still cost time), so that judgement is stated rather than implied. certify_source refuses unless correctness was re-derived under the current source, and report_failures rejects a malformed entry. Editing this file to add the path promptly invalidated its own evidence through the same mechanism, so certification covers the runner sources too. First use: engine source d625ed4a certified against timings measured under 8011ecda, 17 correctness pairs passing, no timing re-measured. Tests: the existing stale-source test passed vacuously once certification existed - it blanked only the measured-under digest while the certification still reached the working tree - so it now clears both. Added a test that a certified source publishes only when it is genuinely unreachable as measured-under, and one that a certification missing its reason, evidence digest or pass count is refused. build_real_strategy_evidence.py joins release_checks.txt, which immediately caught two lint errors it had been escaping by not being in the manifest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPDj1nioTuD4xATMVyhZxB
… source 66 pass, 2 unsupported, the same verdicts as before. Of 231,740 leaf values in the report, 201 differ and every one is a per-scenario duration, an engine digest, a commit or the generation timestamp. No correctness value moved, which is what the LinearImpact.permanent_fraction removal had to demonstrate: a parameter no code path read cannot change an output, and now that is measured across 17 scenarios and 4 frameworks rather than argued from the call site. Unlike the timing evidence, this is a genuine re-derivation and not a re-stamp. Correctness cannot be certified from an inspection of the diff - you do not know behaviour held until you run it - and the matrix is cheap and contention-tolerant, so there is never a reason to skip it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UPDj1nioTuD4xATMVyhZxB
stefan-jansen
deleted the
fix/linear-impact-inert-permanent-fraction
branch
September 11, 2026 23:30
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.
What was wrong
LinearImpact.__init__acceptedpermanent_fractionandcalculate()never read it.LinearImpact(coefficient=0.1, permanent_fraction=0.3)did exactly the same thing asLinearImpact(coefficient=0.1), so a reader followingdocs/user-guide/market-impact.mdcould set it and get silence.Making it real is not a small fix:
calculate()is handed one order and holds no reference to earlier slices of the same parent, so permanent impact has nowhere to accumulate. Removing it or raising on it were the two honest options. (PR #92 takes the deprecation route on the same parameter; these are alternatives, not complements.)The part that cost more than the fix
_tree_digesthashes every tracked.pyundersrc/ml4t/backtest, and that digest is pinned intoREAL_STRATEGY_PERFORMANCE.json. So deleting an unread keyword invalidated the published timings exactly as far as a rewritten fill model would, and regenerating them meansreal_strategy_benchmark.py --samples 10: 374 subprocesses, about 2h29m of engine time, on an otherwise idle machine or the intervals are noise.Spending that to move a hash is not defensible when the numbers cannot have changed.
ml4t_engine_source_sha256now means what it says and is never rewritten: the source the timings were measured under. A later source that moves no measured value goes intocertified_equivalent_sources, and the gate accepts either. The report keeps saying which source produced its numbers while also saying which sources they still apply to.Certification is not a waiver. Each entry carries the correctness evidence it rests on, that evidence's digest and generation time, the passing pair count, the commit, and a required written reason - because correctness parity does not prove a change is performance inert. An added loop can leave every number identical and still cost time, so that judgement is stated rather than implied.
certify_sourcerefuses unless correctness was re-derived under the current source;report_failuresrejects a malformed entry.Editing
real_strategy_benchmark.pyto add this promptly invalidated its own evidence through the same mechanism, so certification covers the runner sources too.Evidence the removal is inert
Both evidence files were re-derived, which is the cheap half and the half that actually proves something:
validation/build_real_strategy_evidence.pyis new:real_strategy_benchmark._commandalready built all 34 invocations correctly, but_measure_sideran them inside aTemporaryDirectorybecause it wants timings, so no output tree survived for the comparator. This driver reuses_commandand_run_onceand retains the outputs. It collects no timings and says so, so a one-sample run is never mistaken for a published measurement.Tests
2155 passed, 9 skipped. The seven previously-red contract tests are green.The existing stale-source test passed vacuously once certification existed - it blanked only the measured-under digest while the certification still reached the working tree. It now clears both. Added one test that a certified source publishes only when it is genuinely unreachable as measured-under, and one that a certification missing its reason, evidence digest, or pass count is refused.
build_real_strategy_evidence.pyjoinsrelease_checks.txt, which immediately caught two lint errors it had been escaping by not being in the manifest.🤖 Generated with Claude Code
https://claude.ai/code/session_01UPDj1nioTuD4xATMVyhZxB