Repository navigation
Store marginal National Insurance schedules as scales - #2490
Conversation
vahid-ahmadi
left a comment
There was a problem hiding this comment.
Fine once item 1 is fixed and the merge order with #2493 and #2452 is settled.
@juaristi22, the migration keeps every value. I compared the 10 legacy paths on main and on this branch: weekly from 2010 to 2035, plus every annual period from 2015 to 2035. That is 1,378 instants per path, and none differs. Reform dicts on the legacy and canonical spellings give identical Class 1 and Class 4 results. A Reform class that calls .update() on a legacy path still works. Baseline NI for a test household matches main.
- must: the bracket-zero leaves are reformable but have no effect. The formulas read only
components[1]andcomponents[2](for exampleni_class_1_employee_primary.py:18-33andni_class_4_main.py:14-27), and nothing reads bracket 0. Yetemployee_scale[0].rate,employee_scale[0].threshold,employer_scale[0].rateand the Class 4 equivalents are ordinary parameters that metadata serialisers will expose. I set each of the three Class 1 ones in a 2026 reform on a £60,000 earner. Employee NI stayed at £3,210.60 and employer NI at £8,250.00, and there was no error. Please either reject reforms to bracket 0 invalidate_ni_scale_compatibility(or in the reform readers), or make the formulas evaluate the whole scale.national-insurance-scales.mdshould also say what bracket 0 is and that it cannot be changed. - should: metadata now lists each NI rate and threshold twice: the 10 legacy views plus 16 scale leaves (27 Class 1 and Class 4 rate or threshold parameters in all, against 11 on main). The page says this is intended. Before merge, someone should check how policyengine-api and the app will show the duplicates, so that users do not see two "NI secondary threshold" controls. The source-extracted API contract tests do not cover the app's parameter tree.
- should:
utils/ni_parameter_scales.pyreassigns__class__on core nodes, swapsnode.childrenfor a filtering dict during processing, and patches uprating metadata in place. Each of these depends on policyengine-core internals that core can change without notice. Because the formulas still read individual components rather than calling the scale, the gain is mostly in layout, and the shim is the cost of that. Please add a short comment at the top of the module saying when the legacy views can be removed, so that this does not become permanent. - should: this conflicts with two of your other open PRs.
git merge-treeshows conflicts with #2493 (ni_class_1_employee_primary.py,ni_class_1_employee_additional.py) and #2452 (create_threshold_indexation.py). Please settle the merge order. Whichever lands second will need its NI parameter reads moved to the scale paths. - nit: the changelog fragment is
changed, which fits. Downstream code that reads the YAML files directly, rather than the parameter tree, will lose the 10 deleted files. A sentence in the fragment would warn those users.
What I checked
- Read the full diff, the PR body and #222.
- Value equivalence: my own script compared the 10 legacy paths on main and on this branch, as above.
- Reform probes on a 2026 household with £60,000 employment income and £30,000 profit: legacy and canonical paths for the main employee rate, the secondary threshold and the upper profits limit, a legacy
Reform.update(), and the bracket-0 probes in item 1. Baseline employee NI of £3,210.60 equals 8% × (£50,270 − £12,570) + 2% × (£60,000 − £50,270). Employer NI of £8,250 equals 15% × (£60,000 − £5,000). pyteston the 6 NI and threshold-indexation test files: 204 passed.policyengine-core testontests/policy/baseline/gov/hmrc/national_insurance: 142 passed.ruff checkandruff format --checkon the 27 changed Python files: clean.- Hosted CI: all 15 checks pass at 28dbe68.
- The Reproducer line is present, and
axiom: n/afits a refactor. I found no overlap with other open PRs beyond #2493 and #2452. - I did not check the law, because no value changes. I did not run the full suite, the API, the app or any microdata.
🤖 Generated with Claude Code
28dbe68 to
e889563
Compare
|
Addressed items 1, 3 and 5 in The exact-commit full Python confirmation returned actual command and pytest exit 0 using Core 3.33.2 / microdf 1.5.11. Focused minimum-version checks remain separate. The original auditor returned 1 for the glibc-only skip on macOS; a separate guarded Darwin assessment returned 0. The earlier run's unavailable OS exit status remains recorded. Items 2 and 4 remain open before merge: deployed API/app parameter-tree, search and duplicate-control acceptance, plus owner-accepted migrations and actual landing across #2464 → #2462 → #2490 and #2493 / #2452. The #2363 measurement migration and liability-curve preservation, #2386 documentation and #2190 ownership conditions also remain open. Combined-stack, current-main and hosted aggregate/data validation remain pending. This PR is not merge-ready; the existing Axiom disposition is unchanged. |
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
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; components the model counts twice; household income totals; population; poverty rates; people in poverty; inequality; mean household net income in each side's own deciles; standard reform costings (change in gov_balance); standard reforms: change in each reform's target. Checks failing on both base and head (1)
Counted-once checkEach of 84 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
Components the model counts twice: 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
|
vahid-ahmadi
left a comment
There was a problem hiding this comment.
@juaristi22 round 2, at e889563e8. The branch was rebased, so I reviewed the new commit e889563e8 against round 1. The rebased first commit carries no other changes.
Verdict: items 1, 3 and 5 are fixed. Items 2 and 4 are still open, as your reply says, so this isn't ready to approve yet.
Round-1 points
- must: bracket-zero leaves: fixed.
_validate_ni_zero_bracketruns inNIParameterScale._get_at_instantand invalidate_ni_scale_compatibility. It rejects any nonzero, missing or null threshold or rate in bracket 0, at any date. I re-ran my round-1 probes on a £60,000 earner in 2026.class_1.employee_scale[0].rate= 0.5 andemployer_scale[0].threshold= 1,000 now raise aValueErrorthat names the leaf. Settingemployee_scale[0].rateto 0 is a no-op (£3,210.60 employee and £8,250.00 employer NI, as in the baseline).employee_scale[1].rateand the legacyrates.employee.mainat 10% both give £3,964.60, which is 10% × £37,700 + 2% × £9,730. The docs now describe bracket 0 as a fixed sentinel. - should: duplicate controls in the API and app: not fixed.
national-insurance-scales.mdnow tells metadata consumers to map each legacy name to its canonical leaf and show one control. Nobody has yet checked what policyengine-api and the app actually do. Someone needs to look at the app's parameter tree and search on a preview before this merges. - should: module comment on retiring the shim: fixed. The docstring now says when the core-internal adaptations and the legacy views can go.
- should: merge order with #2493 and #2452: not fixed.
git merge-treeagainst current heads still conflicts with #2493 (f686e04c1:ni_class_1_employee_primary.py,ni_class_1_employee_additional.py) and #2452 (eeaf911cc:create_threshold_indexation.py). All three PRs are yours, so please pick an order and put it in the bodies. I'd land #2493 and #2452 first. They are smaller, and this PR's legacy views keep their scalar reads working, so the rebase here is mechanical. - nit: changelog warning for direct YAML readers: fixed.
New findings
- nit: overlap with Max's draft #2363. This branch now also conflicts with #2363 in both
salary_sacrifice_pension_ni_*.pyfiles. #2363 is a draft, so this doesn't block, but whichever lands second will need to move its NI reads onto the scale paths. This PR now merges cleanly with #2462 and #2464.
What I checked
- Read the fix commit, your reply and the updated docs.
- Fresh worktree and uv venv at the head.
pytest tests/test_ni_parameter_scales.py: 106 passed.policyengine-core test tests/policy/baseline/gov/hmrc/national_insurance: 142 passed. - Ran the reform probes in item 1 myself. The household is synthetic.
git merge-treeagainst current main (clean), #2462, #2464 and #2469 (clean), and #2493, #2452 and #2363 (conflicts, as above).- CI at the head: all 16 checks pass, and the aggregate-impact report shows no change.
- Not checked: the deployed API or app.
🤖 Generated with Claude Code
|
Documented #2493 → #2452 → #2490 in all three PR bodies, preserving #2464/#2462 and the other recorded dependency conditions. #2452 shares the threshold-indexation helper and tests with this PR. F2 still needs API/app owner-accepted preview evidence for the tree, search and duplicate controls. F4's actual migrations, landing and fresh combined-tree checks remain open; the written order does not complete those gates. The #2363 overlap remains a separate second-landing migration. |
|
Integrated main The genuine local API/app preview on this commit showed all ten canonical controls once and no alias or zero-sentinel controls. Independent code and policy review found no new local issues; validation includes 327 reconciled Python tests, 107 YAML checks and separately reported independent checks. The newer Deployed API/app owner acceptance, actual landing and the existing #2363/#2386/#2190/private-data coordination conditions remain open. The standalone route did not expose search. This PR remains outside the merge queue pending those conditions and current-head human review. |
|
Merged by the UK hub (session local_8e19f9d5, Opus 5.5) on Max's explicit instruction to merge the team's ready PRs before the Budget (21 October, about 23:00Z: "go ahead and merge them. none of us has time to wait on each other"). One merge-queue enqueue, pinned to head
🤖 Generated with Claude Code |
Fixes #222.
Move the ten existing marginal NI rate and threshold histories into three canonical scale parameters: Class 1 employee, Class 1 employer and Class 4. Preserve their values, dates, units, references and policy qualifications, the existing contribution components and input overrides, and the five threshold-indexation writers. Flat Class 2 amounts and liability rules retain their existing structure.
Legacy scalar paths remain serialized compatibility views and support ordinary leaf updates through the existing reform routes. Each view resolves the canonical leaf in its own policy tree. Readers use immutable at-instant bracket components to retain the original arithmetic and dtype, including independently reformed equal or reversed thresholds. The tests cover clone/baseline isolation, cache updates, tracing, generated thresholds, final NI/household paths and targeted errors for conflicting structures.
Bracket zero is the fixed zero-threshold, zero-rate sentinel. The six sentinel histories reject nonzero, missing or null leaves, including future entries; explicit-zero updates remain no-ops. The docs describe this boundary, the module comment states the legacy-view retirement condition, and the changelog warns direct-YAML readers about the ten removed files.
The API compatibility tests execute source-extracted methods and the exact JSON helper from API commit
71dba65b3780f0bb4699d4dcbfe179e5f1506c78. All ten original default scalar IDs, histories, units and non-visibility fields are preserved. Discovery hides the ten legacy aliases and six fixed-zero leaves with household/economy flags; the ten canonical contribution leaves remain visible. Direct legacy lookup still shares canonical metadata and values. Synthetic reserialization after a root update retains an inherited baseline-name overwrite, independently reproduced on the original model commit; live root/alias/canonical values and metadata are checked separately. These are source-extracted contract checks, not a deployed or fully installed API test.Reproducer: policyengine_uk/tests/test_ni_parameter_scales.py::test_ni_zero_band_reforms_are_rejected[dict-class_1.employee_scale-rate]; policyengine_uk/tests/test_threshold_indexation.py::test_canonical_threshold_registration_keeps_incoming_savings_writer; policyengine_uk/tests/test_ni_metadata_compat.py::test_fixed_zero_and_legacy_discovery_controls_are_hidden
Historical validation at published
28dbe681:Historical local model tests used Python 3.13.14, Core 3.32.27, microdf 1.2.1 and Hypothesis 6.168.2; the lock selects Core 3.33.2, microdf 1.5.11 and Hypothesis 6.169.0. Hosted CI must verify the locked environment. That historical validation did not include a full/private-data suite or population-impact claim. The authored base is
e1601dbe4feb15e71d0d59b7a57f9d860741c068. A separate local integration with main62a0117f9a40232d12ecb2c7a20af7441dfed1f1passed all 190 cases: 49 NI cases and 141 incoming pension regressions. The first bounded run completed 102 cases before its time limit; the remaining-only run passed 88 with no repeated passing cases. Two existing pension fixtures also produced identical NI, pension, income-tax and final-cash arrays on actual main and the combined tree.Historical published-fix validation at
e889563e8ad30466a1b040bbb54e2501f835f3ff, based on main7ad002f549680a9ca024c896532697d532a3a92a: the full Python confirmation returned actual command and pytest exit 0 using Core 3.33.2 / microdf 1.5.11, with recorded skips and one expected failure. The original auditor returned 1 for the glibc-only skip on macOS; the separate guarded Darwin assessment returned 0. Earlier failed/interrupted setup records and the earlier run's unavailable OS exit status remain retained. Focused Core 3.32.27 / microdf 1.2.1 compatibility checks passed separately; the whole suite was not run on that minimum environment. These results do not certify private-data, current-main, combined-stack or hosted aggregate validation.Current integration and consumer checks
The local candidate ordinarily integrates captured main
4ed5990ca5485cb40e42737312a9837700b537a9, which already includes #2493 and #2452, and retains the incoming #2464/#2462 NIC-only excess and actual-return contracts. The six mechanical resolutions preserve the constrained 2022–23 director path, CA liability mask, public Core cache lifecycle and eighth savings writer. A separate Pension Credit reader uses the same canonical Class 4 leaves with its existing arithmetic and liability condition.Actual local validation comprises 327 unique Python tests and 107 YAML programme checks across the affected NI, indexation, director, cache, Pension Credit, Carer's Allowance, ESA and salary-sacrifice contracts. Counts reconcile overlapping commands. The first 180-second NI batch remains an incomplete timed-out invocation; its 120 completed cases plus the sole missing case have exact catalogue coverage. A new discovery-identity test initially selected the baseline copy; independently approved current-
govidentity scoping corrected that test only, retaining all API-root record, flag and direct metadata/values assertions. Setup failures and all original outcomes remain retained. Formatting preserved all Python ASTs and non-Python bytes.Independent code and policy/source review of commit
1ea81b8af65eff995a3195770b57ce4fe61295e5found no new local issues. The code reviewer ran 36 independent tests; a later clean integration projection with main7cbdc2b98f3dcfc5f5c24ef4beaab13d0f407b05passed the 13 cases in its newly changed Carer's Allowance property-test file. That main delta changes tests and release metadata, with production code and dependencies unchanged. These counts are separate from the author counts.The genuine local API (
policyengine-api5.6.1, commitcd4039353dcf79171a6bf39241e4c47576d35242) and app (d1df5d1360d916e36740ba22c8b8bcfbc6854b84, Next dev mode) were exercised with this candidate and Core 3.33.2. Dependency checks passed and all loaded UK modules resolved to the candidate. The API retains twenty distinct legacy/canonical IDs, equal full histories and units for all ten pairs, and the intended visibility flags. In the expanded app picker, each of the ten canonical controls rendered once with its baseline value; the ten legacy alias controls and six zero-sentinel leaves were hidden. The standalone route did not expose search, so actual search remains untested. This is local preview evidence with an explicit override of the API lock's Core 3.33.0; it is not a deployed-consumer or production-build certificate.Both local technical review roles completed; the consolidated PR review retains the external F2/F4 conditions below. Current-head hosted CI, deployed consumer-owner acceptance and aggregate/data validation remain required before landing. No full/private-data or population-impact validation is claimed.
Landing coordination
The previously preferred #2493 → #2452 ordering has occurred in captured main; this PR's updated branch has not yet landed. F2 remains open until API/app owners accept the candidate preview. F4 remains open until the owner-accepted shared-component migrations land and fresh combined checks are accepted. #2363's owner must migrate excess measurement to
salary_sacrifice_pension_ni_excess, preserve actual-return semantics and retain liability-difference curves whichever change lands first. #2386 documentation, #2190 labour-supply ownership and other private-data/consumer conditions remain open. This PR is not merge-ready. No encoding or parity completion is claimed.axiom: n/a — structural parameter refactor preserving the existing rules, histories and calculation behavior; no new policy encoding or encoder run.