Skip to content

Fix construction reform wrappers after parameter edits - #2485

Merged
juaristi22 merged 2 commits into
mainfrom
fix-2480-construction-reform-wrappers
Oct 20, 2026
Merged

juaristi22 merged 2 commits into
mainfrom
fix-2480-construction-reform-wrappers

Conversation

@juaristi22

Copy link
Copy Markdown
Collaborator

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=True for 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.

@vahid-ahmadi vahid-ahmadi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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, not Scenario.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.
  2. nit: docs/book/usage/scenarios.md:428-432 is 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.
  3. nit: #2193 also edits policyengine_uk/simulation.py. It adds an apply_reform override about 30 lines above your first hunk. git merge-tree shows the two branches merge cleanly, and they change different behaviour. Whichever lands second should still run test_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.py and utils/scenario.py to 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_state attribute. They are not all behaviour checks.
  • ruff check and ruff format --check on the 3 changed Python files: clean.
  • Hosted CI: all 15 checks pass at 92b6a30.
  • The changelog fragment is fixed. The Reproducer line is present, and axiom: n/a fits 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 vahid-ahmadi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, per my review above. The nits can follow.

@juaristi22
juaristi22 added this pull request to the merge queue Oct 19, 2026
Merged via the queue into main with commit 7bcdb45 Oct 20, 2026
15 checks passed
@juaristi22
juaristi22 deleted the fix-2480-construction-reform-wrappers branch October 20, 2026 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-ups from #2434's review: construction-wrapper own-dict boundary, docs and changelog

2 participants