Skip to content

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

Closed

Description

@MaxGhenis

Follow-up from the UK hub's round 4 review of #2434 (approved with nits at 2ffa1b13e). None of these blocks #2434, and none is a regression from main.

1. A construction-time wrapper call equal to the scenario's own dict keeps a stale value (P2)

  • policyengine_uk/utils/scenario.py:99 treats any DatedReform equal to one of the scenario's own as "own", and takes the construction reading without an effectiveness proof.
  • scenario.py:228 and :241 disable the composite proof and the full replay while _building_dated_reforms is set (simulation.py:260).

Reproducer. Let RATE = gov.hmrc.income_tax.rates.uk[0].rate, A = {RATE: {"2027-01-01.2027-12-31": .31}} and L = {RATE: {"2027-01-01": .35}}.

  1. Build Simulation(scenario=Scenario.from_reform(A) + Scenario(simulation_modifier=wrapper)).
  2. Inside wrapper, call Scenario.from_reform(L).apply(sim), then Scenario.from_reform(A).simulation_modifier(sim), or the composite (L, A).

The modifier leaves .35 on 1 January 2027, where .apply gives .31. A direct edit followed by a request for A leaves a stale .40. No error or warning is raised. On #2434's head, 4 of 12 such cases mismatch; on main, all 12 do.

Possible fix: treat an equal dict as "own" only while no foreign override or assignment has been logged on its paths since construction applied its parameters.

Care is needed: equality-based ownership is load-bearing for policyengine.py's composer (provenance/receipt.py, scenario_composition/v1), which runs each member's from_reform modifier inside its own wrapper. A naive identity or value check would also change behaviour for deep-copied scenarios and for LCWRA amounts that rebalancing overwrites.

Add a test pinning whichever boundary is chosen.

2. Docs and changelog

  • docs/book/usage/scenarios.md:421–422: state finding 1's boundary until it is fixed. Also say that mixed composites always replay, which rewrites a copy's own construction values. For example, {RATE: {"2026-06-01.2027-03-31": .31}, VAT: {"2027-01-01": .25}} on a clone of a simulation built with it moves the 2027 rate from .20 to .31.
  • Dated measures on indexed thresholds need Simulation(scenario=...). On a baseline-built simulation, the modifier and apply follow the annualised reading and can't express Resume indexation from the changed amount after a dated change to an indexed threshold #2423's indexation resume. For example, a PA freeze to 5 April 2031 returns to 13,090 in 2032.
  • changelog.d/2426.fixed.md: add a bullet. Scenario.apply now raises for keys covering no 1 January ("2027-04-06", "2027-04-06.2027-12-31", "2027-02-01.2027-12-31"), where main silently applied them from that date.

3. A test for relative reforms on copies

The review reply says "relative-reform clone/branch controls" passed, but no such test exists. The reviewer's probes passed 12 of 12: a Reform class, a modifier scaling the PA by 1.1, and a CPI growth-rate dict. Add them as a test.

Evidence: the UK hub's review files, reviews/uk2434/review-r4.md (local to the hub).

Activity

  1. self-assigned this
    on Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions