Repository navigation
Fix construction reform wrappers after parameter edits - #2485
Merged
Merged
Conversation
vahid-ahmadi
left a comment
Collaborator
There was a problem hiding this comment.
Looks right, fine to approve.
@juaristi22, this closes the stale-value case from #2480 item 1 and covers items 2 and 3. Ownership now needs both an equal dict and untouched paths. You check the paths against a snapshot of each timeline plus the two write logs, so a foreign write with the same value but a different date reading still counts as foreign. The state is cleared at the end of __init__, so clones made after construction carry None and behave as on main.
- nit:
utils/scenario.py:178. A construction-time replay (construction_replay) logs_warn_annualised_overrides(), and that message starts "Scenario.apply edits the built simulation's annualised parameters". In this path the user called a modifier inside a constructor wrapper, notScenario.apply, so the text may confuse them. A short message for this case, such as "a construction wrapper edited these paths, so this reform overrides the annualised values", would be clearer. - nit:
docs/book/usage/scenarios.md:428-432is accurate but dense. The paragraph on what happens during construction would read better as three or four bullets: owned and untouched, owned but edited, external composite, and the data-load boundary. - nit: #2193 also edits
policyengine_uk/simulation.py. It adds anapply_reformoverride about 30 lines above your first hunk.git merge-treeshows the two branches merge cleanly, and they change different behaviour. Whichever lands second should still runtest_construction_reform_wrappers.py, because both touch how reforms bind to a cloned system.
What I checked
- Read the full diff, the PR body and #2480.
pytest test_construction_reform_wrappers.py: 43 passed. On the existing reform-route tests (test_dated_reform_routes.py,test_from_reform_modifier.py,test_scenario_sum_timing.py,test_reform_representations.py): 251 passed.- Reverted
simulation.pyandutils/scenario.pyto main: the Reproducer test fails in all 8 parametrisations, and 22 of the 43 new tests fail overall. Some of these fail on main only because they assert the new_construction_reform_stateattribute. They are not all behaviour checks. ruff checkandruff format --checkon the 3 changed Python files: clean.- Hosted CI: all 15 checks pass at 92b6a30.
- The changelog fragment is
fixed. The Reproducer line is present, andaxiom: n/afits an engineering-only change. - I did not run policyengine.py's composer itself. Like the PR, I relied on the composer-shaped fixtures in the new tests.
🤖 Generated with Claude Code
vahid-ahmadi
approved these changes
Oct 18, 2026
vahid-ahmadi
left a comment
Collaborator
There was a problem hiding this comment.
Approving, per my review above. The nits can follow.
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.
Fixes #2480.
A construction callback could overwrite a parameter and then request a reform equal to the constructor's original dictionary, leaving the overwritten value in place. For example, requesting a 2027 rate of 0.31 could retain the callback's 0.35 or 0.40. Construction ownership now checks the affected parameter timelines and write logs before retaining the original reading. Edited or external requests replay using the built simulation's annualised reading, including edits outside the original date window and across data loading.
Construction-time clones and isolated policy branches also keep separate ownership checkpoints, so child-only edits preserve the original simulation. The change preserves content-equal and deep-copied scenarios, ordered composer-style callbacks, original legacy-write order and intentional UC rebalancing shadowing. Documentation explains constructor versus built/copy readings, mixed-composite replay, indexed-threshold resume and
clone_system=Truefor independent policy branches. The changelog also records the inherited rejection of override keys containing no 1 January.Validation: 294 distinct cases passed, including 43 new regressions and 251 affected existing cases. Formatting, lint, whitespace and protected-file checks passed, with Python ASTs unchanged by formatting. The documentation build and rendered-page checks passed without notebook execution.
The offline runtime uses Python 3.13.14, Core 3.32.27 and microdf 1.2.1, compared with locked Core 3.33.2 and microdf 1.5.11. No dependencies were installed and no private dataset was used. Composer compatibility was checked against its captured source contract and equivalent public wrapper fixtures; its package was not executed.
Reproducer: policyengine_uk/tests/test_construction_reform_wrappers.py::test_equal_constructor_wrapper_after_edit_matches_apply
axiom: n/a: reform execution and provenance engineering; no statutory policy provision, numerical policy parameter or eligibility rule changes.