Repository navigation
Fix the uniform decile property reporting domain - #2511
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
|
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 #2511 at 38cdaa4. No must or should findings, so approving. Three nits below, none blocking.
What I checked
- Reproduced the #2500 failure on the old strategy: with
seed=0, shift=0.005, deciles 4 and 10 come out suppressed (1 to 9 households cross> 0.005through float noise), somean_changeisNoneand thepytest.approxrow assertion fails. The comparator (policyengine_uk/utils/aggregate_impact/compare.py:318) is unchanged, as the issue asked. - Swept the new domain (whole-pound incomes, shifts of ±1, ±50, ±99, ±100, ±101 pence, 300 seeds): no row was suppressed or reported
any_changefalse. Whole pennies on whole pounds keep clear of both the half-penny and the £1 boundaries. - Checked the boundary cases by hand against the comparator: zero base income makes the change exactly the shift, so
nextaftereither side of 0.005 and the exact ±£1 cases test the strict>comparisons as written. policyengine_uk/tests/aggregate_impact/: 237 passed, 9 skipped locally. CI is green at head. Changelog fragment andaxiom: n/a: ...line are fine for a tests-only change.
Findings
-
nit
policyengine_uk/tests/aggregate_impact/test_aggregate_impact_compare.py:665-668. The property is called "shows in every decile" and the new comment calls it a visibility property, but nothing in it asserts visibility. For any shift under 50p,mean_changerounds to 0 and both shares are 0, so the loop would still pass if every row saidany_change: False. Suppression is caught only indirectly (Nonefailsapprox). The new half-penny test does pinany_change, so this is not a gap in the file, but addingassert not row["suppressed"]andassert row["any_change"]to the property would make it test what its name says. My sweep above suggests both hold on the new domain. -
nit Merge order with your #2506. Both PRs append tests at the end of
test_aggregate_impact_compare.py, andgit merge-treereports a content conflict there. It should be a simple keep-both resolution for whichever lands second. -
nit The branch is 42 commits behind
main. CI is green at head, so a rebase is only worth doing if #2506 lands first and you resolve item 2 anyway.
🤖 Generated with Claude Code
Collaborator
Author
|
Integrated captured main The combined comparison file passed all 133 cases locally, with no failures, errors or skips. Ruff, the scoped diff check, Towncrier and the trusted main Fix evidence check passed. The original 118-case validation in the PR body describes the earlier head; it is not an additional 118 cases on this integration. No production formulas, reporting thresholds, privacy rules or dependencies were changed by the conflict resolution. |
vahid-ahmadi
approved these changes
Oct 22, 2026
vahid-ahmadi
left a comment
vahid-ahmadi
left a comment
Collaborator
There was a problem hiding this comment.
@juaristi22 second look at #2511, now at e8543be (your merge of main 68ce6a6 into 38cdaa4). Approving again.
What I checked
- The merge kept your change exactly. Against main
68ce6a6, the PR now touches onlychangelog.d/2500.fixed.mdandtest_aggregate_impact_compare.py, and its added and removed lines match the diff I approved at38cdaa4line for line. The only difference is where the blank lines sit, because your two boundary groups now come before the #2506 group rather than at the end of the file. - Nothing from main was dropped. The only lines removed relative to main are the three old property lines your change replaces. #2506 (merged as
7a8750c) is in the branch history and its data-axis runtime-identity tests are all still in the file. - Tests at head: the comparison file passes all 133 cases, matching your count, and
policyengine_uk/tests/aggregate_impact/gives 252 passed, 9 skipped. Main has moved on since your merge. A local merge of current main into the head merges cleanly and gives the same 252 passed, 9 skipped. - CI at
e8543be: all checks are green, including Test 1 to 4, YAML, Lint, Fix evidence and Aggregate impact (no change in any household's net income).
Earlier findings
- nit, the property does not assert visibility: still open, not blocking.
test_aggregate_impact_compare.py:664-667would still pass if every row saidany_change: False. Addingassert not row["suppressed"]andassert row["any_change"]would make it test what its name says. Fine to leave for another time. - nit, merge order with #2506: fixed. #2506 landed first and you kept both groups.
- nit, branch behind main: fixed by the merge.
🤖 Generated with Claude Code
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 #2500
The uniform-change property admitted half-penny shifts even though the reporting contract intentionally treats changes at that boundary as unchanged. Generate nonzero integer-penny shifts and whole-pound baseline incomes for this property, and add deterministic cases at the half-penny and £1 reporting boundaries. The comparator, privacy rules, numeric tolerances and the other property tests are unchanged.
Validation: the original
seed=0, shift=0.005counterexample failed the old numeric-row assertion; the repaired property plus new boundary cases passed all 12 focused cases, and the complete affected comparison test file passed all 118 cases.make format, Ruff F811/F821 and the trusted base Fix check passed. No model or private-data calculations were needed.Reproducer: policyengine_uk/tests/aggregate_impact/test_aggregate_impact_compare.py::test_a_uniform_change_shows_in_every_decile; policyengine_uk/tests/aggregate_impact/test_aggregate_impact_compare.py::test_uniform_change_respects_half_penny_reporting_boundary; policyengine_uk/tests/aggregate_impact/test_aggregate_impact_compare.py::test_uniform_change_respects_one_pound_gain_loss_boundary
axiom: n/a: Tests-only infrastructure fix; no policy formulas, parameters, thresholds or dependencies change.