Repository navigation
Require complete runtime identity for aggregate-impact data comparisons - #2506
Merged
Merged
Conversation
Contributor
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
|
juaristi22
marked this pull request as ready for review
October 19, 2026 19:51
vahid-ahmadi
approved these changes
Oct 20, 2026
vahid-ahmadi
left a comment
vahid-ahmadi
left a comment
Collaborator
There was a problem hiding this comment.
@juaristi22 first-round review of #2506 at 7a5b559. The fix is right and I have no must or should findings. Three nits below, none blocking.
What I checked
compare.py:736-737addsruntime.pythonandruntime.machineto the data-axis identity map in_same_model_problems, so the existingif not b or not hbranch now catches absent,Noneand""values. That closes #2499: before this,_runtime_differences(compare.py:400-406) only compared the two sides, so missing on both sides "agreed".- In a fresh worktree,
pytest policyengine_uk/tests/aggregate_impactgives 241 passed, 9 skipped. - With the base
compare.pyswapped in, the 15issue_2499cases give 7 failed, 8 passed, matching the PR body. The 7 are the both-sides cases and the null/empty cases. The one-side cases already failed on base through_runtime_differences, so they are controls, not regressions, which is how the body describes them. - The model axis is unchanged: Python and machine differences there are still warnings only (
compare.py:494-498). - CI is green at head. The
Reproducer:andAxiom: n/alines are present, andchangelog.d/2499.fixed.mdis there.
Findings
-
nit: a Python or machine mismatch on the data axis now gives two failure lines for one cause. With head on
3.99.0you get "needs one Python version, but the sides have 3.13 and 3.99.0" fromcompare.py:747-753, and then "needs one runtime, but base and head differ in: python: 3.13 -> 3.99.0" fromcompare.py:757-761. A missing machine on one side also appears twice ("does not record it" and "machine: arm64 -> None"). This does no harm, but the report reads as two problems. One fix is to droppython/machinefrom_runtime_differenceswhen it is called for the data axis (compare.py:512), so the identity map is the only place that reports them. -
nit: when both sides lack a field, the message names only the base (
compare.py:749-751:'the base' if not b else 'the head'). That is existing behaviour shared with the other identity fields, but #2499 is exactly the both-sides case, so "neither side records it" would be more accurate. Optional. -
nit: merge order with #2511. Both PRs append tests at the end of
test_aggregate_impact_compare.py, aftertest_withheld_decile_years_are_explained_not_called_missing.git merge-treeof the two heads gives a content conflict in that file. It is a trivial append conflict, so whichever merges second needs a rebase.
Outside this PR's scope
The stale-base path in the Aggregate impact workflow is still open. aggregate-impact.yaml:69,77 takes the base from github.event.pull_request.base.sha, not from the first parent of $GITHUB_SHA, although the comment at :75-76 says they are the same. On 19 Oct, on #2433, base.sha (62a0117f9a) lagged the test-merge commit's first parent (7726c1a176), and the base crashed with KeyError: 'war_widows_pension'. This PR's own run was not affected: merge commit ae7a43ba0f has first parent 0b8af54cac, equal to base.sha. The PR does not touch the workflow, so I'm not holding it on this. Using git rev-parse "$GITHUB_SHA^1" after checking out the head would close it. I couldn't find an issue for it, so it probably needs one of its own.
Approving, since there are no must or should findings and CI is green.
🤖 Generated with Claude Code
This was referenced Oct 20, 2026
MaxGhenis
added a commit
that referenced
this pull request
Oct 21, 2026
…ment-charge No conflicts. programs.yaml auto-merged: main's registry fixes (#2443) and this branch's notes on income tax, the Winter Fuel Payment and PAWHP touch different entries. #2493's 2022 full-year director NI and #2506's aggregate-impact runtime identity read nothing this branch changes; the director tests read income_tax only in 2022, before the charge. Main: 6990350 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 #2499
Data-axis aggregate-impact comparisons now require a Python version and machine identity in both measurements. Previously, two omitted fields compared equal and allowed an incomplete comparison to pass. Missing or empty values now fail through the existing identity checks.
The tests cover each field and both fields missing from either or both measurements, null and empty values, complete data comparisons, and the existing model-axis behavior. Policy formulas, dataset pins, thresholds and privacy controls are unchanged.
Reproducer: policyengine_uk/tests/aggregate_impact/test_aggregate_impact_compare.py::test_issue_2499_data_axis_requires_runtime_identity_on_each_side
Axiom: n/a: aggregate-impact reporting metadata validation only; no policy formula, parameter or eligibility change.
Validation: the 15 focused cases produced 7 failures and 8 passing controls before the fix, then all 15 passed. The combined data-free aggregate-impact and workflow batch passed all 247 cases.
make formatand focused F811/F821 lint passed.The certified population tests and full model suite were not run for this metadata-only change. No survey data or policy-impact measurement was needed.