Repository navigation
Hold BSP flat after the dataset benefit split - #2644
Conversation
vahid-ahmadi
left a comment
There was a problem hiding this comment.
@juaristi22 thanks, this is my review at e760e4c20.
Verdict. The approach is right, and better than the one I proposed in #2381. Keying the freeze on the dataset keeps legacy files on CPI, so the £84m error the issue warned about cannot happen for today's data. The law is right and the new tests do what the body says. Two shoulds: the trigger is the allowance column alone, which still lets a dataset freeze War Widow's Pension, and two docs pages now say the opposite of what the code does. CI is not finished yet (the Python test shards and Aggregate impact are still pending), so this is a comment, not an approval.
A mistake of ours first. Step 3 of #2381 said to drop bsp_reported from the CPI list and remove its uprating attribute once released datasets carry the split. That would have frozen every dataset, including the Enhanced FRS and older Microcosm releases that still hold codes 6 and 9 together. That is the £84m error again, just later. Your dataset-keyed design avoids it, and the class-level CPI declaration has to stay for legacy files. The issue text was wrong on this.
What I checked
- Law. SI 2017/410 reg 3 still reads £350, £3,500, £100 and £2,500. SI 2026/148 art 19 says each "remains unchanged", and art 18 raises Widowed Parent's Allowance from £150.90 to £156.65. The GOV.UK BSP page shows the same amounts. So the body's line "rates remain unchanged under article 19 of the 2026 up-rating order" is accurate, and art 19 is the right citation.
- Producer. At microcosm#1383 head
e570d65ff,frs_spine.pywriteswidowed_parents_allowance_reportedfor every person from_sum_to_entity(amount * wpa, ...), so a person with no allowance gets 0. The name matches exactly, the column is in the frame's person column list and in the terminal coverage list, andwar_widows_pension_reportedis written alongside it. - Legacy path. With no allowance column,
bsp_reportedkeeps CPI in the projector, after the horizon and when economic assumptions are off (test_missing_wpa_column_preserves_legacy_cpi_uprating, legacy and partial-split cases). A calculated default allowance does not switch it on (test_calculating_default_wpa_does_not_migrate_a_legacy_dataset). - Tests. At the head,
test_bsp_dataset_projection.py,test_projection_horizon.py,test_widowed_parents_allowance.py,test_war_widows_pension.py,test_uprating_inputs_reach_data.py,test_benefit_uprating_published.pyand the code-health uprating, simulation-method, changelog, input-definition and spending tests: 228 passed. Against the merge baseb1fb356cc, the new file gives 25 failed and 12 passed. The 12 that pass are the legacy and household cases, which should pass on both. Both Reproducer tests fail on the base for the positive and zero allowance cases. - Edge cases in
simulation.py:1363-1401. The fallback is skipped for a formula or neutralised override, for a non-CPIuprating, for situations and for periods that are themselves inputs. It takes the latest earlier annual input and reads the allowance input for that same period. Monthly output is divided once. Reforms and baselines both see the split. I found no case where it misfires. - Merge order. The branch merges cleanly with main (4 commits ahead, none in these files). Of the open PRs touching the same files, #2627, #2635, #2638, #2639 and #2265 merge cleanly with this head. #2589, #2131, #2002, #2077 and #1969 already conflict with main, so nothing new comes from this PR.
Findings
- should,
policyengine_uk/simulation.py:1394-1399,policyengine_uk/data/economic_assumptions.py:345-349and:855-858. The split is declared bywidowed_parents_allowance_reportedalone.test_bsp_dataset_projection.py:123(test_wpa_column_alone_signals_the_completed_split) fixes that on purpose with no war-widow column. But a dataset that splits the allowance out before it moves FRS code 9 still has War Widow's Pension inbsp_reported, about 90% of its value in the raw FRS. policyengine-uk-data#578 asks for the code 9 move first, so this needs that order to be reversed or a third-party dataset, but nothing in the model stops it. Such a dataset would freeze the pension, which is the £84m case from #2381. This is a cheap guard rather than a likely path. Requiring bothwar_widows_pension_reportedandwidowed_parents_allowance_reportedcosts nothing for Microcosm, which writes both. That test would then flip to keep CPI. - should,
docs/book/programs/gov/dwp/bereavement-support-plan.md:56-66still saysbsp_reported"keeps CPI uprating only until the datasets move FRS code 9 out of it and WPA intowidowed_parents_allowance_reported", that microcosm#1222 is open, and that #2381 "tracks freezing it after that". After this PR that is out of date.docs/book/assumptions/growthfactors.md:23also listsbsp_reportedamong the CPI variables without the exception. A short line pointing to the new section indataset-sources.mdwould fix both. - nit,
policyengine_uk/variables/gov/dwp/war_widows_pension.py:18-20says "the datasets still add it tobsp_reported(policyengine-uk-data#578, microcosm#1222)". For Microcosm that stopped with microcosm#1333. It would be worth correcting while you are in this area. - nit,
docs/engineering/skills/dataset-sources.md:138-139. Off-rate amounts kept in splitbsp_reportedare held flat. microcosm#1383 says these include an allowance with an inherited additional pension above the allowance rate, which rises with CPI. In the raw FRS 2024-25 the whole off-rate bucket is about £7m a year, so the effect is small. Saying in one clause that this part is understated would make the guide complete.
Aggregate effect. On today's released data, which has no allowance column, nothing changes, so the Aggregate impact job should show zero. For a release built from microcosm#1383, the change removes CPI growth from BSP alone. Using the model's CPI path from a 2024 base (cumulative factor 1.058 in 2026-27, 1.079 in 2027-28, 1.100 in 2028-29 and 1.122 in 2029-30) on the roughly £100m a year of raw FRS 2024-25 code 6 at BSP rates, spending is lower by about £6m in 2026-27, £8m in 2027-28, £10m in 2028-29 and £12m in 2029-30, or up to about £13m if all off-rate amounts are included. A calibrated release will give a somewhat different base.
PR body and changelog. The Reproducer tests exist and fail on the base. The axiom: n/a reason fits a projection change with no change to rates or rules. changelog.d/2381.fixed.md describes the change accurately.
🤖 Generated with Claude Code
Aggregate impact on certified data: passBase Checks: 0 broken by this change, 1 failing on both sides, 0 fixed. Moves flagged for review: 0 (thresholds in Notes
Moves flagged for reviewNone. Spending by benefit and service
Caseloads
Standard reform costings (change in gov_balance)
Standard reforms: change in each reform's target
Household net income by decileNo household's net income changes in any year. No change in any year: receipts by tax head; government balance, each component counted once; model totals; household income totals; population; poverty rates; people in poverty; inequality; mean household net income in each side's own deciles. Checks failing on both base and head (1)
Counted-once checkEach of 87 components was raised by £1 per unit in 2026-27, and every head of its totals was read household by household: its own head must rise by exactly £1, no other tax head may move, no other benefit may rise, and the model's own total must move by what its heads moved. A head that breaks a rule is measured again twice: with every other head held at its baseline value, and with every other head held at its value after the first £1 while the component rises again. It fails where either still shows the response. 0 failed; 0 were skipped.
Provenance
Dataset: Receipts by tax head: head levels
Spending by benefit and service: head levels
Caseloads: head levels
Government balance, each component counted once: head levels
Model totals: head levels
Household income totals: head levels
Population: head levels
Poverty rates: head levels
People in poverty: head levels
Inequality: head levels
Mean household net income in each side's own deciles: head levels
Standard reform costings (change in gov_balance): head levels
Standard reforms: change in each reform's target: head levels
|
e760e4c to
72daf28
Compare
|
Addressed all four findings from the review:
The guard regressions reproduced 12 failures before the fix. Tests now cover positive and zero complete pairs, both partial schemas, latest-observation provenance, the 4 × 4 reset matrix and branch invalidation for either input. Microcosm #1383 has merged and writes both required inputs unconditionally. Rebased cleanly onto The class-level BSP CPI declaration remains in place for legacy datasets. No statutory rate or entitlement rule changes. |
|
@juaristi22 thanks, this is my second review, at Round 1 findings
New findings None. What I checked
I did not run the full suite or build a Microcosm dataset with #1383, and I did not read the unchanged parts of the docs pages again. Verdict. All four round 1 findings are resolved and I found nothing new. CI is green at this head, including all five Python shards and Aggregate impact. From my side this is ready to merge. 🤖 Generated with Claude Code |
vahid-ahmadi
left a comment
There was a problem hiding this comment.
Approving at 72daf28; see the round 2 comment above.
🤖 Generated with Claude Code
Fixes #2381
Hold reported Bereavement Support Payment (BSP) nominally flat when a dataset explicitly supplies both
war_widows_pension_reportedandwidowed_parents_allowance_reportedin the same source year. All-zero columns count as supplied inputs. Microcosm's merged #1383 writes both columns. BSP rates remain unchanged under article 19 of the 2026 up-rating order.Older datasets can combine BSP with CPI-linked War Widow's Pension and Widowed Parent's Allowance. They retain CPI uprating, including datasets that separate only one benefit. Household situations retain their existing CPI behaviour. The separate allowance and war-widow inputs continue to grow with CPI, and the class-level BSP CPI declaration remains available to legacy consumers.
Apply the same two-input contract during automatic projection, in the Simulation/Microsimulation fallback after the materialised horizon, and when economic assumptions are disabled. The fallback checks both explicit inputs in the source period of the latest supplied BSP amount. Calculated defaults and inputs in different periods do not declare a completed split. Supplied annual amounts remain authoritative; disabling economic assumptions preserves a later BSP observation when either observation contains the complete split.
The published BSP plan, growth-factor page, dataset-source guidance and variable metadata describe this behaviour. The guide also records the producer's residual limitation: unclassified off-rate amounts can include Widowed Parent's Allowance with inherited additional pension, so holding that CPI-linked component flat understates it.
Reproducer:
policyengine_uk/tests/test_bsp_dataset_projection.py::test_completed_split_freezes_bsp_in_each_projected_year;policyengine_uk/tests/test_bsp_dataset_projection.py::test_completed_split_freezes_bsp_after_the_projection_horizon;policyengine_uk/tests/test_bsp_dataset_projection.py::test_wpa_column_alone_preserves_legacy_cpi_upratingThe new guard regressions failed against the prior implementation: 12 failed and 19 passed before the fix. The fixture-scale tests use controlled CPI paths without private survey data. They cover complete and partial schemas, positive and zero inputs, same-period provenance, the 4 × 4 reset matrix, computed defaults, dependency invalidation for either input, monthly conversion, authoritative multi-year amounts, reform/baseline isolation, clones and rebuilds, structural overrides and cache opt-out.
axiom: n/a: dataset projection and backward-compatibility correction; statutory rates and entitlement rules are unchanged.
Validation:
mainatadbaa87d7(282.13 seconds).uv run --frozen --no-sync policyengine-core test policyengine_uk/tests/policy/baseline/gov/dwp/widowed_parents_allowance.yaml -c policyengine_uk: 7 passed after rebasing.uv run --frozen --no-sync make format; post-rebaseruff format --check .andruff check .pass.uv run --frozen --no-sync make documentationpassed before and after rebasing.The full certified-data suite was not run locally. CI runs the repository suite on the pushed head.
Targeted Python validation command