[MaxGhenis]

Fixes #618

Reform.modify_parameters deep-copied baseline.parameters although Reform.__init__ had already cloned them. That copy threw away what apply() had changed before it, and it carried the baseline's cached nodes with it, so an edit that bypassed Parameter.update could read the baseline's values. In-place reforms had the same stale read.

What changes

• Reform no longer overrides modify_parameters. It inherits TaxBenefitSystem.modify_parameters, so a modifier gets the reform's own tree on both paths a reform runs.
• TaxBenefitSystem.modify_parameters drops every node cached at an instant once the modifier finishes, even if it raises: in the tree it handed over, in any tree the modifier returned, and in the system's own cache. It rebinds that system cache rather than clearing it, as it did before, because policyengine-us shares the dict between SPM policies.
• ParameterNode.clear_descendant_caches does the walk, brackets included. A loaded bracket has no parent, so Parameter.update clears the bracket and nothing above it.

The cache design stays as it is, for #621. This PR changes when modify_parameters clears caches, and nothing else about them. That includes one change on the in-place path: a modifier that returns something other than a tree now resets the system cache too, where master returned before resetting it.

Behaviour changes for reforms built as instances

• Changes made earlier in apply() stay, and successive modify_parameters calls build on each other, as apply_reform_set already did.
• A modifier that edits the tree in place and returns something else keeps its edits, again as apply_reform_set did. modify_parameters still returns a ValueError here rather than raising it.
• A reform instance can pass the dict form.
• modify_parameters on a reform instance returns the reform, not None, as TaxBenefitSystem.modify_parameters does. The annotation now says so.
• Edits a modifier made before raising stay in the reform's tree, which apply() sees if it catches the exception. With one update and a raise, master's instance read 600 and the branch reads 1, as apply_reform_set does on both.
• A modifier sees parent set on brackets, because clones set it and a deep copy of a loaded tree left it None. An update inside a bracket now clears caches up to the root on this path.
• Narrower: a modifier used to get a private deep copy, so it could edit nested metadata or a list value in place. It now gets a clone, which shares those with the baseline (Share nested metadata values between parameter clones #631), so such an edit would reach the baseline. A modifier has to replace them. The docstring says so, a test pins replacing them, and a search of policyengine-us, policyengine-uk and the API found no reform that edits them in place. ct_hb5114 replaces its list through update.

Direct edits with no later modify_parameters, such as Reform.from_dict applied in place to a loaded tree's bracket, can still read stale values. That stays with #621.

Invariants

For any baseline read at any instants, and any reform whose apply() edits the tree directly, reads it, calls modifiers ending in any way and uses the dict form, on either path:

• Fresh reads: once modify_parameters has run last, the system, its tree and each bracket read as a tree never read at all, given the same edits. That holds for a modifier that raises as well.
• The two paths agree: an instance and apply_reform_set leave the same tree.
• The baseline stays put: an instance changes nothing in its baseline, its values, its metadata or the nodes it has cached.
• Composition: two modifiers do what one modifier making both sets of edits does.
• Differential: for one modifier on a baseline never read, the tree matches what the old deep copy left. Only this case can match, because the deep copy dropped earlier steps and read stale values.

Two exclusions are deliberate: the direct edit above, and in-place edits of nested metadata or list values.

Tests

• tests/core/test_reform_modify_parameters.py: 49 examples, including the template reform on both paths, warm and cold edits, brackets through the dict form, returned and dropped trees, a raising modifier, the return value, the rebound system cache and reforms of reforms.
• tests/core/test_reform_modify_parameters_property.py: 6 Hypothesis properties, 200 derandomized examples each.
• 40 of these 55 tests fail on master.
• All 21 mutants of the fix fail the suite: each cache left uncleared, partial walks, a walk that reaches siblings, the walk or the reset moved before the modifier, the system cache cleared rather than rebound, the deep copy restored, None returned and edits rolled back on a bad return. Reviewers predicted the 4 that first survived, and the tests that catch them are in this PR. A 22nd mutant cloned the tree after the edits, which keeps them, so it matched the fix and I dropped it.
• The fresh-read property reaches the stale state. With the walk removed, 16 of 116 instance examples and 25 of 84 in-place examples read differently from a tree never read. With the system cache kept, 11 and 45 do.
• test_reforms.py, test_vectorial_parameter_node_copy.py and test_tracing_parameter_isolation.py no longer describe the deep copy or clear caches by hand.

Differential against master

Same model commits on both sides: policyengine-us 2.90.2 at 469ad12cb9, policyengine-uk 2.170.0 at 2da212abc, master at 3c89556f2.

policyengine-us, 85 cases: the Reform objects in the 25 modules that use modifiers, as an instance and in place, the contrib switches, and hand-written bracket and rate reforms. Each case compares the tree digest and household outputs.

• 84 match. 78 of those changed parameters, and 2 fail the same way on both sides, with VariableNameConflictError from ut_hb210_s2.
• 1 differs, and master was wrong: set_parameter on a bracket as an instance. The trees match, but master's household still read baseline tax (income tax 1,712). The branch reads -688, which is what the in-place path reads on both Cores.
• The baseline tree stays unchanged after every instance reform.

policyengine-uk, 25 cases:

• 23 match, and 21 of those had parameter effects.
• 2 differ, both intended:
  • A modifier that assigns values as an instance kept master at baseline household income while its tree changed. The branch reads the change.
  • Two dict modifiers as an instance raised TypeError on master and now work, matching the in-place result.
• The baseline tree stays unchanged.

Test suites on this branch:

• policyengine-us YAML, the 27 files that name a reform that reaches this code: all pass, 276 cases.
• policyengine-us Python tests in the 37 files that touch reforms: 674 pass and 1 fails, in 35 files that pass, one that fails on both Cores (below) and one fixture module with no tests.
• policyengine-uk reform test files, 44 files: all 1,421 tests pass.

Timing

Building a Reform on policyengine-us (156,949 nodes, Python 3.14.7). Median of 5 rounds, wall / CPU seconds, master and branch running side by side on a loaded host.

Build	Master	Branch
Modifier, tree never read	27.6 / 15.1	3.9 / 2.3
Modifier, read at 32 instants	50.1 / 30.1	5.1 / 2.8
Modifier, never read, GC paused	10.7 / 8.3	0.52 / 0.47
Modifier, read at 32 instants, GC paused	22.3 / 17.0	0.79 / 0.59
set_parameter, read at 32 instants	54.3 / 30.6	4.6 / 2.8
No modifier, tree never read	4.1 / 2.2	4.1 / 2.3

The YAML runner pauses GC while it builds a reform (#643), which is what the two paused rows show. The no-modifier row is the clone both Cores share. Peak memory for the run falls from 5.0 GB to 2.6 GB.

The in-place path did no walk on master, so it gets slower:

• policyengine-us: a modify_parameters call on a tree read at 16 instants costs 0.13 s of CPU, up from nothing measurable.
• policyengine-uk: building a household Simulation makes 6 calls and costs 2.01 s of CPU, against 2.00 s on master. A walk of the tree takes a median 1.71 ms.

Not run

The full policyengine-us and policyengine-uk suites: I ran the affected files only, at ea99d5d5f. The commit after it changes only tests, a docstring and a return annotation. The one US failure, TestSocialSecurityInputStateMachine, fails the same way on both Cores here, because it looks for Core's files in site-packages and my install is editable. CI's Test Core and country packages job passes.

axiom: n/a: engine change in Core, no country policy.

🤖 Generated with Claude Code