Repository navigation
Conversation
test_ordinary_parameters_read_every_key_as_before drew its ordinary parameters from every gov leaf outside yoy_growth, so it also drew the energy price indices, which read calendar years. A pull request that moves the derandomized sample onto one of them fails the check. The ordinary list and the calendar-year check now share one prefix tuple, and a new test asserts that no ordinary parameter reads calendar years. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
juaristi22
reviewed
Nov 10, 2026
juaristi22
left a comment
juaristi22
left a comment
Collaborator
There was a problem hiding this comment.
Review status: COMPLETE at 1dba0719f51c58720d11e7c374f55d705b06894b. No critical findings or material evidence gaps; one should-address item remains.
A1 — Add the required typed changelog fragment. This diff changes only policyengine_uk/tests/test_process_time_reforms.py and adds no changelog.d/<slug>.<type>.md. AGENTS.md and docs/engineering/skills/github-prs.md require a typed Towncrier fragment for test changes as well as other non-dependency-only changes. Please add a short fragment describing the corrected calendar-series sampler. The current checker emits a warning for this omission; this is a nonblocking repository-standard finding, not a test or model defect.
The four focused tests passed, including independent metadata-classifier coverage, the new non-calendar ordinary invariant, the configured 200-example differential check, and the Simulation/reform fiscal-year parameter check. In this exact tree, 2,257 ordinary leaves remain after excluding the 60 growth and 19 energy-price leaves; dotted prefixes preserve boundary behavior. The locked runtime and snapshot import were verified.
Approval remains deferred until A1 is resolved and applicable required checks pass at the exact current head. Hosted Python/Aggregate CI was still pending at the latest review capture.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
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; 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 checkThe check raised each of 92 components by £1 per unit in 2026-27 and read every head of its totals 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 response that is not finite breaks every rule. The check measures a head that breaks a rule twice more: once holding every other head at its baseline value, and once holding every other head at its value after the first £1 while the component rises again. The head fails where either measurement still shows the response. The check cannot hold a head at a value that is not finite. In each unit where another head is not finite after the first £1, it holds that head at its baseline value, and the second measurement judges no household the unit belongs to. There a head fails wherever it first broke a rule, or wherever the check could not confirm whether its response contains the component. 0 failed and the check skipped 0.
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
|
juaristi22
reviewed
Nov 10, 2026
juaristi22
left a comment
juaristi22
left a comment
Collaborator
There was a problem hiding this comment.
Review status: COMPLETE
Reviewed head: 6b3d7db90bdda37d153ece4c57d0b8a5fca69e32
A1 is resolved: changelog.d/fix-ordinary-calendar-year-sampling.fixed.md is a valid, nonempty typed fragment and describes the sampler fix. The description and author reply match it. There are no remaining findings or material evidence gaps.
The incremental diff adds only that fragment. All 4,097 prior Git entries—including the test module, model, parameters and dependency lock—are unchanged, and the prior technical evidence remains valid. I retained the four passing Python tests from the previous review; no tests or model calculations were rerun for this changelog-only commit. No policy/source change was introduced.
Approval is deferred while exact-head CI is running. I will check the required results on a later round without repeating the completed code review.
juaristi22
approved these changes
Nov 11, 2026
juaristi22
left a comment
juaristi22
left a comment
Collaborator
There was a problem hiding this comment.
Approve at 6b3d7db90bdda37d153ece4c57d0b8a5fca69e32. The retained completed review is COMPLETE with no remaining findings or material evidence gaps. All 21 latest exact-head CI contexts passed, including Lint and Test / Result, and this PR is Ready.
Retained source, test/runtime receipts and durable evidence were integrity-checked. No code review, tests or model runs were repeated. The head, description, human activity, source/parent guards, readiness and required checks are refreshed immediately before this approval.
This branch has not been deployed
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.
Problem
test_ordinary_parameters_read_every_key_as_beforechecks invariant 4 ofpolicyengine_uk/tests/test_process_time_reforms.py: a reform to a parameter that is not a calendar-year series reads every key as before. It drew those parameters from everygovleaf outsidegov.economic_assumptions.yoy_growth. The household energy price indices (gov.economic_assumptions.energy_prices) became calendar-year series in 7c69b0d, and the test kept drawing from them: on main at 3cf10f8, 19 of the 2,276 paths it draws from read calendar years.Hypothesis derandomizes the draw, so the parameter tree decides which paths the 200 examples reach. On main's tree none of them reaches an energy price path. A pull request that adds or removes parameters moves the draw and can land on one. #2861's Python 1/8 shard failed this way at 4788937:
With the calendar-year check, a bare 2016 reads as calendar year 2016; without it, as fiscal year 2016-17. A calendar-year series should read the calendar year, so the model has no bug here; the test drew a parameter outside its own invariant.
Change
CALENDAR_YEAR_SERIES, holds thegovprefixes of the calendar-year series: the growth rates, the indices and the energy prices.test_calendar_year_series_are_the_growth_rates_indices_and_household_treeand the ordinary list both read that tuple, so a calendar-year series added to one reaches the other.test_no_ordinary_parameter_reads_calendar_years, asserts that no ordinary parameter reads calendar years.The change touches only the test module.
Tests
Reproducer: policyengine_uk/tests/test_process_time_reforms.py::test_no_ordinary_parameter_reads_calendar_years
-k "ordinary or calendar_year_series_are"passes 4 of 4, including all 200 examples of the differential check.ruff format --checkandruff checkpass.changelog.d/fix-ordinary-calendar-year-sampling.fixed.mdrecords the fix.🤖 Generated with Claude Code