Repository navigation
Bind a subsampled reform's rebuilt baseline arm to baseline policy - #9994
Merged
Merged
Conversation
Core's subsample rebuilds the baseline arm as a branch of the reform and then hands it the replaced arm's policy. Before core 3.32.21 it does not rebind the branch's holders, so the baseline read the reform's neutralized income_tax ($0 instead of $4,016), and the arm kept the replaced arm as its own baseline. No core version points the policy back at the new arm or primes its private parameter-tracing root, so a traced baseline arm wrote its tracer onto the shared parameter tree (core 3.32.8). Clear the arm's baseline, rebind its holders and isolate its tracing, as construction does. Adds a regression test for both wrappers and their clones and a Hypothesis property that subsampling never changes which policy a holder belongs to. Found in review of #9698 (P3-1). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sampling A reform's baseline arm is a branch of the reform, so its holders were built for the reform's variables. Rebinding one to the baseline's variable kept the holder's population and storage, so a reform that moved a variable to another entity, or into or out of ETERNITY, left the baseline arm with storage its own variable cannot use (core 3.32.8; core 3.32.21 drops these holders itself). Drop them with their recorded inputs, as core 3.32.21's binder does, on both baseline-arm paths. apply_reform keeps a simulation's own inputs, as core's does. No shipped reform changes a variable's entity or ETERNITY-ness. Tests now also check that each holder suits its variable, and the property test's structural reform redefines is_household_head as yearly. The regression test reforms the supplied system instead of rebuilding policy from source. Addresses review r1 of #9994. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Hand-off to the US + core hub
|
Contributor
Author
|
US + core hub merge audit, us#9994 at
|
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.
Summary
Microsimulation(reform=...).subsample(...)(andSimulation(dataset=..., reform=...).subsample(...)) rebuilt the baseline arm still bound to the reform. This PR binds the rebuilt arm to its own baseline policy, the way construction binds a reform simulation's baseline arm.Found in the round-3 review of #9698 (finding P3-1). It does not depend on #9698:
_rebind_holdersand_isolate_parameter_tracingare already onmain(policyengine_us/spm.py).Root cause
Core's
Simulation.subsamplerebuilds the baseline arm withself.get_branch("baseline"), a branch of the reform simulation, and then assigns it the replaced arm's tax-benefit system. Read in the installed core packages:uv.lockpins.subsampleassigns the system and nothing else.clonecopies every instance attribute exceptbranches,trace,tracer,debugand_fast_cache, so the new arm's ownbaselineis the arm it replaced, on the population from before subsampling.labor_supply_behavioral_responseandcapital_gains_responsescompute responses wheneversimulation.baseline is not None, so the baseline arm would measure responses against that stale arm.uv lock --upgrade-package policyengine-corebefore testing.subsamplenow callsbaseline._bind_to_tax_benefit_system()and setsbaseline.baseline = None, which fixes the holder bindings. That binder also drops holders the reform built in another entity's population, or with storage for (or not for) an ETERNITY variable; core 3.32.8 keeps them, at construction as well as insubsample. It cannot do the US-only part:simulationback-reference at the new arm;_isolate_parameter_tracingskips unlesspolicy.simulation is self.Observed on
main(2 earners at $50,000,UserReformfromtest_spm_system.py, which neutralizesincome_taxand replaceshousehold_market_income, with tracing turned on after construction, thensubsample(2)):income_taxbaseline.tax_benefit_system.simulation is baselinebaseline.baseline is Nonesystem.parameters.traceafter a baseline calculationFullTraceris written onto the process-wide treeFix
In
SPMSimulationMixin.subsample, after core rebuilds the arm:This is what
__init__gives a reform's baseline arm, and what #9698's_rebuild_baseline_branchdoes.New
_drop_holders_built_for_another_policy(). It applies core 3.32.21's compatibility rule, so results no longer depend on the core version:__init__now runs it on the baseline arm too, where core 3.32.8 had the same gap;apply_reform, which (like core's) keeps a simulation's own inputs.All the calls are idempotent after core 3.32.21's own binding.
Does core own this? Core's generic part is already done: core #587, released in 3.32.21, rebinds holders and clears the arm's baseline. What remains is this package's own state (the policy's
simulationback-reference and the private tracing root), so the fix belongs in the US wrapper. No core PR is needed for it.Tests
All in
policyengine_us/tests/core/test_spm_system.py, next to the existing subsample test (SPM CI group):test_subsampled_reform_baseline_and_clones_calculate_original_tax, run for bothSimulationandMicrosimulation. Aftersubsample:income_taxis $4,016 and the reform's is $0;household_market_incomeis $50,000 in the baseline and $123 in the reform;delete_arrays, and are bound to their own policy.The $4,016 matches what the existing
test_reform_baseline_and_clones_calculate_original_taxasserts for an ordinary, unreformed simulation of the same households.test_subsampling_never_changes_which_policy_a_holder_belongs_to, a Hypothesis property test:n,seedandquantize_weights. The reform kinds are:is_household_headas yearly;max_examples=2plus one explicit example, withderandomize=True, matching the repo's other simulation-backed Hypothesis tests. The explicit example drives both deletion paths: the reform-only variable, and the redefined variable's yearly holder. A guard asserts that both holders came through the rebuild.Fails on main, passes here, run locally and labelled by core version:
income_tax$0, $4,016 expected; the property test's explicit example finds the stalebaseline.baseline). Existing subsample test passes.policy.simulationis the replaced arm). Existing subsample test passes.is_household_headkeeps yearly storageInvariants
The property test covers the first two; the regression test covers all three.
Impact
No formula, parameter or variable changes. The new lines run only for a reform simulation's baseline arm, in
subsampleand at construction.bypassoverride 191 baseline variables when applied, and none changes entity or ETERNITY-ness (script:scan_reforms.py, run 2026-10-07).bypass). Review r2 checked those statically. Their overrides keep entity and period, and Trustees changes parameters only.So nothing they compute changes, and no microsim run is needed.
subsamplecalls in policyengine.py, policyengine-api, policyengine-api-v2 or policyengine-household-api. In this repo the only callers are tests, and the only ones that subsample a reform simulation are theRUN_HEAVY_TESTS=1-gatedtest_behavioral_structural.pyandtest_lsr_cg_interaction.py.simulation.baseline is Noneguards in the labour-supply and capital-gains responses now skip responses there. On core ≥ 3.32.21 values were already right, and the change is the policy back-reference and tracing isolation._behavioral_response_measurementscache in_invalidate_all_caches, whichsubsamplecalls; without it, measurements taken before subsampling survive it.CI cost
The new tests run in the SPM group (
make test-other-python-spm, the Microsimulation runner). No job, matrix entry or group was added.test_spm_system.pytakes 302 s.test_reform_baseline_and_clones_calculate_original_tax(36–42 s per wrapper on CI);simulation-test-timingsartifact.Interactions
_rebuild_baseline_branch(the same pattern, at construction) and editstest_spm_system.pyat a different spot. There's no textual overlap. Whichever lands second should addrebuilt._drop_holders_built_for_another_policy()beforerebuilt._rebind_holders()there._isolate_parameter_tracingin favour of core's per-call tracing. Whichever lands second should drop thebaseline._isolate_parameter_tracing()line added here; the regression test's tracing block will flag it.Not fixed here (follow-up filed)
Core's
subsampleexportsto_input_dataframe(include_computed_variables=True), which core's own docstring calls "the historical unsafe behavior". Anything the reform arm calculated before subsampling becomes an input of the rebuilt population, in both arms. For example, with a standard-deduction-to-zero reform, calculatingtaxable_incomeand then subsampling leaves the baseline reading the reform's $48,755 instead of $35,400. That is a separate core defect, filed as a follow-up. The property test therefore checks bindings, not values, after pre-subsample calculations.axiom: n/a: simulation infrastructure (subsample wiring); no policy provision changes.
🤖 Generated with Claude Code