From ff0b130167fd86613de1e108a1433f51fb8cfbc1 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 4 Oct 2026 21:37:09 -0400 Subject: [PATCH 1/3] Stop targeting OBR total NICs on ni_employee MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cash-receipts parser targeted Table 3.8 "National insurance contributions" (total NICs, £200.08bn in 2025-26) on the ni_employee variable, which _parse_nics also targets at the Class 1 employee figure (£49.54bn). The two targets on one matrix column contradicted each other; the seeded constituency calibration on main b45c373 ended with obr/ni at 0.242 of target. Drop the row rather than retarget it: the Table 3.4 class targets (employee, employer, self-employed) already cover every NIC class the data populates, and a total target on total_national_insurance would conflict with them through the statutory payment recoveries and "Other NIC" lines PE-UK does not model. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk_data/targets/sources/obr.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/policyengine_uk_data/targets/sources/obr.py b/policyengine_uk_data/targets/sources/obr.py index ad4e3cf1..7da502e7 100644 --- a/policyengine_uk_data/targets/sources/obr.py +++ b/policyengine_uk_data/targets/sources/obr.py @@ -182,6 +182,7 @@ def _parse_receipts(wb: openpyxl.Workbook) -> list[Target]: the standard fiscal forecasting convention. Other receipts use the current-receipts table (cash basis) since they only appear there; that table is located by title because EFO vintages renumber the sheets. + NICs are not read here: ``_parse_nics`` targets them by class. """ config = load_config() vintage = config["obr"]["vintage"] @@ -237,9 +238,18 @@ def read_39(ws, row_num: int) -> dict[int, float]: # located by title rather than number: EFO vintages renumber the tables, and # in March 2026 current receipts moved to 3.8 while 3.9 became the APD # forecast, which silently yielded no targets at all. + # + # The "National insurance contributions" row is deliberately not targeted. + # It is total NICs, and it used to be targeted on ni_employee, which + # _parse_nics also targets at the Class 1 employee figure (£200bn against + # £50bn on one variable in 2025-26). Retargeting it on + # total_national_insurance would still conflict: the total also counts + # statutory payment recoveries and "Other NIC" (Class 1A, 1B and 3, + # settlements, unallocated). PE-UK has no variable for the first three, and + # no dataset fills Class 3 (#378). The class targets already cover every + # NIC class the data populates. ws39 = _find_receipts_sheet(wb) cash_rows = { - "ni": ("National insurance contributions", "ni_employee"), "vat": ("Value added tax", "vat"), "fuel_duties": ("Fuel duties", "fuel_duty"), "capital_gains_tax": ("Capital gains tax", "capital_gains_tax"), From 64d07fc07c130135ce4ed83b2a2f2cc16d53c0a7 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 4 Oct 2026 21:37:09 -0400 Subject: [PATCH 2/3] Test OBR target variables and shared-column agreement - test_obr_target_mapping: pin the variable of every target parsed from the committed receipts workbook, the Table 3.4 rows behind the NIC class targets, and the source identity that rules out a total-NICs target. - test_obr_target_conflicts: no two calibration targets whose matrix column is the same plain variable sum or count may disagree on a year's value, checked over the OBR targets offline and over every target the national matrix uses; it flags the removed obr/ni mapping, and a seeded random check holds the grouped check to its pairwise definition. - build_loss_matrix.calibration_targets: the target list create_target_matrix already built inline, extracted so the test checks the same set. Co-Authored-By: Claude Opus 5.5 --- changelog.d/obr-ni-total-target.fixed.md | 1 + .../targets/build_loss_matrix.py | 30 ++-- .../tests/test_obr_receipts_sheet.py | 6 +- .../tests/test_obr_target_conflicts.py | 156 ++++++++++++++++++ .../tests/test_obr_target_mapping.py | 103 ++++++++++++ 5 files changed, 281 insertions(+), 15 deletions(-) create mode 100644 changelog.d/obr-ni-total-target.fixed.md create mode 100644 policyengine_uk_data/tests/test_obr_target_conflicts.py create mode 100644 policyengine_uk_data/tests/test_obr_target_mapping.py diff --git a/changelog.d/obr-ni-total-target.fixed.md b/changelog.d/obr-ni-total-target.fixed.md new file mode 100644 index 00000000..21af4b12 --- /dev/null +++ b/changelog.d/obr-ni-total-target.fixed.md @@ -0,0 +1 @@ +Stop calibrating OBR total NICs (Table 3.8, £200bn in 2025-26) on `ni_employee`, which the Class 1 employee NICs target (£50bn) also used; the two targets on one matrix column could not both be met. The Table 3.4 class targets (employee, employer, self-employed) already cover every NIC class the data populates. Tests now pin each OBR target's variable and check that no two calibration targets sharing a plain-variable column disagree on a year's value. diff --git a/policyengine_uk_data/targets/build_loss_matrix.py b/policyengine_uk_data/targets/build_loss_matrix.py index 9552aeea..81ebbaa8 100644 --- a/policyengine_uk_data/targets/build_loss_matrix.py +++ b/policyengine_uk_data/targets/build_loss_matrix.py @@ -61,6 +61,22 @@ logger = logging.getLogger(__name__) +def calibration_targets() -> list[Target]: + """The national, regional and country targets the national matrix uses.""" + targets = [] + seen = set() + for level in ( + GeographicLevel.NATIONAL, + GeographicLevel.REGION, + GeographicLevel.COUNTRY, + ): + for t in get_all_targets(geographic_level=level): + if t.name not in seen: + seen.add(t.name) + targets.append(t) + return targets + + def create_target_matrix( dataset, time_period: str = None, @@ -89,23 +105,11 @@ def create_target_matrix( ctx = _SimContext(sim, time_period, dataset, reform) - all_targets = [] - seen = set() - for level in ( - GeographicLevel.NATIONAL, - GeographicLevel.REGION, - GeographicLevel.COUNTRY, - ): - for t in get_all_targets(geographic_level=level): - if t.name not in seen: - seen.add(t.name) - all_targets.append(t) - df = pd.DataFrame() target_names = [] target_values = [] - for target in all_targets: + for target in calibration_targets(): try: val = _resolve_value(target, year) if val is None: diff --git a/policyengine_uk_data/tests/test_obr_receipts_sheet.py b/policyengine_uk_data/tests/test_obr_receipts_sheet.py index d8b2928d..a3b5a443 100644 --- a/policyengine_uk_data/tests/test_obr_receipts_sheet.py +++ b/policyengine_uk_data/tests/test_obr_receipts_sheet.py @@ -122,7 +122,8 @@ def test_raises_when_rows_are_found_but_yield_no_values(monkeypatch): def test_parses_rows_when_values_are_present(monkeypatch): - """The same sheet with values yields one target per cash-basis row.""" + """The same sheet with values yields one target per cash-basis row, except + total NICs, which the NICs parser targets by class instead.""" monkeypatch.setattr( "policyengine_uk_data.targets.sources.obr.load_config", lambda: {"obr": {"vintage": "test", "efo_receipts": "https://example.invalid"}}, @@ -130,4 +131,5 @@ def test_parses_rows_when_values_are_present(monkeypatch): targets = _parse_receipts(_receipts_wb(populate=True)) names = {target.name for target in targets} assert "obr/capital_gains_tax" in names - assert len(names) == 5 + assert "obr/ni" not in names + assert len(names) == 4 diff --git a/policyengine_uk_data/tests/test_obr_target_conflicts.py b/policyengine_uk_data/tests/test_obr_target_conflicts.py new file mode 100644 index 00000000..192cd471 --- /dev/null +++ b/policyengine_uk_data/tests/test_obr_target_conflicts.py @@ -0,0 +1,156 @@ +"""Calibration targets that share a matrix column must agree. + +A target whose column is a plain sum (or recipient count) of one variable +shares that column with every other such target on the same variable. Two of +them with different values for one year contradict each other: no weights +meet both, so the optimiser splits the difference and distorts everything +else. obr/ni (total NICs, £200bn in 2025-26) and obr/ni_employee (£50bn) did +exactly that on ni_employee. + +The column a target gets is decided by ``build_loss_matrix._compute_column``, +so these tests ask that dispatcher rather than restating its rules: every +custom compute function is swapped for a sentinel, so only targets that reach +the plain-variable fallbacks get a column key. +""" + +import math +import random +from unittest.mock import patch + +import pytest +import requests + +from policyengine_uk_data.targets import build_loss_matrix as blm +from policyengine_uk_data.targets.schema import Target, Unit +from policyengine_uk_data.targets.sources import obr + +# Targets allowed to share a plain-variable column with different values: the +# frozenset of their names, mapped to the reason. None are intended today. +INTENDED_SHARED_COLUMNS: dict[frozenset[str], str] = {} + + +@pytest.fixture +def column_of(monkeypatch): + """Map a target to ("sum" | "count", variable, countries), or None.""" + for name in dir(blm): + if name.startswith("compute_"): + monkeypatch.setattr(blm, name, lambda *args, **kwargs: None) + monkeypatch.setattr(blm, "_compute_simple_gbp", lambda t, ctx: ("sum", t.variable)) + monkeypatch.setattr( + blm, "_compute_simple_count", lambda t, ctx: ("count", t.variable) + ) + + def column(target): + if target.custom_compute is not None: + return None + key = blm._compute_column(target, None, None) + if key is None: + return None + # Targets restricted to different countries get different columns + # (the ``countries`` field proposed in #490 and #530). + countries = getattr(target, "countries", None) + return key + (tuple(countries) if countries else None,) + + return column + + +def conflicts(targets, column) -> set[tuple]: + """(column, year, names) for every shared column whose values disagree.""" + groups: dict[tuple, list[Target]] = {} + for target in targets: + if (key := column(target)) is not None: + groups.setdefault(key, []).append(target) + found = set() + for key, group in groups.items(): + for year in {y for t in group for y in t.values}: + values = { + t.name: v + for t in group + if (v := blm._resolve_value(t, year)) is not None + } + first = next(iter(values.values()), None) + if any(not math.isclose(v, first, rel_tol=1e-9) for v in values.values()): + names = frozenset(values) + if names not in INTENDED_SHARED_COLUMNS: + found.add((key, year, names)) + return found + + +@pytest.fixture(scope="module") +def obr_targets(): + obr._download_workbook.cache_clear() + + def get(*args, **kwargs): + raise requests.ConnectionError("offline") + + with ( + patch.object(obr.requests, "get", side_effect=get), + patch.object(obr.time, "sleep", lambda s: None), + ): + targets = obr.get_targets() + obr._download_workbook.cache_clear() + return targets + + +def test_obr_targets_agree_on_shared_columns(obr_targets, column_of): + assert conflicts(obr_targets, column_of) == set() + + +def test_total_nics_on_ni_employee_is_flagged(obr_targets, column_of): + """The check fails on the mapping this PR removed.""" + total_nics = Target( + name="obr/ni", + variable="ni_employee", + source="obr", + unit=Unit.GBP, + values={2025: 200.08e9}, + ) + found = conflicts(obr_targets + [total_nics], column_of) + assert {(key, names) for key, _, names in found} == { + (("sum", "ni_employee", None), frozenset({"obr/ni", "obr/ni_employee"})) + } + assert 2025 in {year for _, year, _ in found} + + +def test_calibration_targets_agree_on_shared_columns(column_of): + """Every national, regional and country target the national matrix uses.""" + assert conflicts(blm.calibration_targets(), column_of) == set() + + +def _brute_force(targets, column) -> set[tuple]: + """Reference: compare every pair of targets directly.""" + found = set() + for i, a in enumerate(targets): + for b in targets[i + 1 :]: + if column(a) is None or column(a) != column(b): + continue + for year in set(a.values) | set(b.values): + va, vb = blm._resolve_value(a, year), blm._resolve_value(b, year) + if None not in (va, vb) and not math.isclose(va, vb, rel_tol=1e-9): + found.add((column(a), year)) + return found + + +def test_conflict_check_matches_pairwise_definition(column_of): + """Seeded random target sets: the grouped check flags exactly the + (column, year) pairs a pairwise comparison does. Covers custom columns, + counts against sums, and the nearest-earlier-year fallback.""" + rng = random.Random(0) + for _ in range(300): + targets = [] + for i in range(rng.randint(0, 8)): + is_count = rng.random() < 0.3 + years = rng.sample(range(2020, 2027), rng.randint(1, 3)) + targets.append( + Target( + name=f"synthetic/{i}", + variable=rng.choice(["a", "b", "c"]), + source="synthetic", + unit=Unit.COUNT if is_count else Unit.GBP, + is_count=is_count, + values={y: rng.choice([1.0, 2.0]) for y in years}, + custom_compute=(lambda *a: None) if rng.random() < 0.2 else None, + ) + ) + grouped = {(key, year) for key, year, _ in conflicts(targets, column_of)} + assert grouped == _brute_force(targets, column_of) diff --git a/policyengine_uk_data/tests/test_obr_target_mapping.py b/policyengine_uk_data/tests/test_obr_target_mapping.py new file mode 100644 index 00000000..65643059 --- /dev/null +++ b/policyengine_uk_data/tests/test_obr_target_mapping.py @@ -0,0 +1,103 @@ +"""Pin which PE-UK variable each OBR receipts target calibrates. + +The cash-receipts parser once targeted total NICs ("National insurance +contributions", Table 3.8) on ``ni_employee``, the variable the NICs parser +targets at Class 1 employee NICs alone. These tests read the committed EFO +workbooks offline and pin the receipts targets' variables, the rows the NIC +targets come from, and why total NICs has no target of its own. +""" + +from unittest.mock import patch + +import openpyxl +import pytest +import requests + +from policyengine_uk_data.storage import STORAGE_FOLDER +from policyengine_uk_data.targets.sources import obr + +# Every target parsed from the receipts workbook. Expenditure-side and static +# OBR targets are left to their own tests. +EXPECTED_VARIABLES = { + "obr/income_tax": "income_tax", + "obr/vat": "vat", + "obr/fuel_duties": "fuel_duty", + "obr/capital_gains_tax": "capital_gains_tax", + "obr/sdlt": "stamp_duty_land_tax", + "obr/ni_employee": "ni_employee", + "obr/ni_employer": "ni_employer", + "obr/ni_self_employed": "ni_self_employed", +} + +# Table 3.4 rows under "National insurance contributions" in the committed +# workbook: the class targets, then the rows the model leaves empty. +CLASS_ROWS = { + "obr/ni_employee": "Class 1 Employee NICs", + "obr/ni_employer": "Class 1 Employer NICs", + "obr/ni_self_employed": "Class 4 and Class 2 Self employed NICs", +} +UNMODELLED_ROWS = ("Statutory payment recoveries", "Other NIC") + + +@pytest.fixture(scope="module") +def offline_targets(): + obr._download_workbook.cache_clear() + + def get(*args, **kwargs): + raise requests.ConnectionError("offline") + + with ( + patch.object(obr.requests, "get", side_effect=get), + patch.object(obr.time, "sleep", lambda s: None), + ): + targets = obr.get_targets() + obr._download_workbook.cache_clear() + return {t.name: t for t in targets} + + +@pytest.fixture(scope="module") +def receipts(): + return openpyxl.load_workbook( + STORAGE_FOLDER / "obr_efo" / "efo_receipts.xlsx", data_only=True + ) + + +def _row_2025(ws, label: str, column: str) -> float: + """Value for 2025-26 (in £) of the row whose label starts with ``label``.""" + return ws[f"{column}{obr._find_row(ws, label)}"].value * 1e9 + + +def test_receipts_target_variables_are_pinned(): + wb = obr._fallback_workbook("efo-receipts") + targets = obr._parse_receipts(wb) + obr._parse_nics(wb) + assert {t.name: t.variable for t in targets} == EXPECTED_VARIABLES + + +def test_total_nics_row_exists_but_has_no_target(offline_targets, receipts): + """The absence comes from the mapping, not from a missing row.""" + cash = obr._find_receipts_sheet(receipts) + total = _row_2025(cash, "National insurance contributions", "E") + assert total > 150e9 + assert "obr/ni" not in offline_targets + assert all(t.values.get(2025) != total for t in offline_targets.values()) + + +@pytest.mark.parametrize("name,label", CLASS_ROWS.items()) +def test_nic_class_targets_read_table_3_4(offline_targets, receipts, name, label): + expected = _row_2025(receipts["3.4"], label, "D") + assert offline_targets[name].values[2025] == pytest.approx(expected) + + +def test_class_targets_and_unmodelled_rows_make_up_total_nics(receipts): + """Why total NICs is not targeted on total_national_insurance. + + The class targets plus rows the model leaves empty (statutory payment + recoveries; Class 1A, 1B and 3 in "Other NIC") sum to the Table 3.4 total, + so a total target would demand NICs no household carries. + """ + ws = receipts["3.4"] + total = _row_2025(ws, "National insurance contributions", "D") + classes = sum(_row_2025(ws, label, "D") for label in CLASS_ROWS.values()) + unmodelled = sum(_row_2025(ws, label, "D") for label in UNMODELLED_ROWS) + assert unmodelled > 0 + assert classes + unmodelled == pytest.approx(total, abs=0.02e9) From 22ef313669da4ca24256b25c5c15a698dea51bcf Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 20:59:30 -0400 Subject: [PATCH 3/3] Drop obr/ni from the fallback test set; key conflicts on countries - test_obr_efo_fallback: the receipts and NICs set from #536 listed obr/ni, which this branch no longer emits. - test_obr_target_conflicts: #490/#530 added Target.countries, and create_target_matrix zeroes a column outside them, so the column key now includes countries. The random differential check uses Hypothesis, now a dev dependency, and draws country restrictions too. Co-Authored-By: Claude Opus 5.5 --- changelog.d/obr-ni-total-target.fixed.md | 2 +- .../tests/test_obr_efo_fallback.py | 2 +- .../tests/test_obr_target_conflicts.py | 68 +++++++++++-------- 3 files changed, 43 insertions(+), 29 deletions(-) diff --git a/changelog.d/obr-ni-total-target.fixed.md b/changelog.d/obr-ni-total-target.fixed.md index 21af4b12..f0e4f4cf 100644 --- a/changelog.d/obr-ni-total-target.fixed.md +++ b/changelog.d/obr-ni-total-target.fixed.md @@ -1 +1 @@ -Stop calibrating OBR total NICs (Table 3.8, £200bn in 2025-26) on `ni_employee`, which the Class 1 employee NICs target (£50bn) also used; the two targets on one matrix column could not both be met. The Table 3.4 class targets (employee, employer, self-employed) already cover every NIC class the data populates. Tests now pin each OBR target's variable and check that no two calibration targets sharing a plain-variable column disagree on a year's value. +Stop calibrating OBR total NICs (Table 3.8, £200bn in 2025-26) on `ni_employee`, which the Class 1 employee NICs target (£50bn) also used; the two targets on one matrix column could not both be met. The Table 3.4 class targets (employee, employer, self-employed) already cover every NIC class the data populates. Tests now pin the variable of each OBR receipts target and check that no two calibration targets sharing a plain-variable column disagree on a year's value. diff --git a/policyengine_uk_data/tests/test_obr_efo_fallback.py b/policyengine_uk_data/tests/test_obr_efo_fallback.py index 5884642c..5ea3267d 100644 --- a/policyengine_uk_data/tests/test_obr_efo_fallback.py +++ b/policyengine_uk_data/tests/test_obr_efo_fallback.py @@ -95,9 +95,9 @@ def _connection_error(): _EFO_URL_KEYS = ["efo_receipts", "efo_expenditure"] +# Total NICs (obr/ni) is no longer a target: _parse_nics targets NICs by class. _RECEIPTS_AND_NICS_TARGETS = { "obr/income_tax", - "obr/ni", "obr/vat", "obr/fuel_duties", "obr/capital_gains_tax", diff --git a/policyengine_uk_data/tests/test_obr_target_conflicts.py b/policyengine_uk_data/tests/test_obr_target_conflicts.py index 192cd471..62db663f 100644 --- a/policyengine_uk_data/tests/test_obr_target_conflicts.py +++ b/policyengine_uk_data/tests/test_obr_target_conflicts.py @@ -14,11 +14,12 @@ """ import math -import random from unittest.mock import patch import pytest import requests +from hypothesis import HealthCheck, given, settings +from hypothesis import strategies as st from policyengine_uk_data.targets import build_loss_matrix as blm from policyengine_uk_data.targets.schema import Target, Unit @@ -46,10 +47,9 @@ def column(target): key = blm._compute_column(target, None, None) if key is None: return None - # Targets restricted to different countries get different columns - # (the ``countries`` field proposed in #490 and #530). - countries = getattr(target, "countries", None) - return key + (tuple(countries) if countries else None,) + # create_target_matrix zeroes a column outside the target's countries, + # so targets restricted to different countries get different columns. + return key + (target.countries,) return column @@ -131,26 +131,40 @@ def _brute_force(targets, column) -> set[tuple]: return found -def test_conflict_check_matches_pairwise_definition(column_of): - """Seeded random target sets: the grouped check flags exactly the - (column, year) pairs a pairwise comparison does. Covers custom columns, - counts against sums, and the nearest-earlier-year fallback.""" - rng = random.Random(0) - for _ in range(300): - targets = [] - for i in range(rng.randint(0, 8)): - is_count = rng.random() < 0.3 - years = rng.sample(range(2020, 2027), rng.randint(1, 3)) - targets.append( - Target( - name=f"synthetic/{i}", - variable=rng.choice(["a", "b", "c"]), - source="synthetic", - unit=Unit.COUNT if is_count else Unit.GBP, - is_count=is_count, - values={y: rng.choice([1.0, 2.0]) for y in years}, - custom_compute=(lambda *a: None) if rng.random() < 0.2 else None, - ) +@st.composite +def synthetic_targets(draw) -> list[Target]: + targets = [] + for i in range(draw(st.integers(0, 8))): + is_count = draw(st.booleans()) + years = draw( + st.lists(st.integers(2020, 2026), min_size=1, max_size=3, unique=True) + ) + targets.append( + Target( + name=f"synthetic/{i}", + variable=draw(st.sampled_from(["a", "b", "c"])), + source="synthetic", + unit=Unit.COUNT if is_count else Unit.GBP, + is_count=is_count, + countries=draw(st.sampled_from([None, ("ENGLAND",), ("WALES",)])), + values={y: draw(st.sampled_from([1.0, 2.0])) for y in years}, + custom_compute=(lambda *a: None) + if draw(st.integers(0, 4)) == 0 + else None, ) - grouped = {(key, year) for key, year, _ in conflicts(targets, column_of)} - assert grouped == _brute_force(targets, column_of) + ) + return targets + + +@settings( + max_examples=300, + deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture], +) +@given(targets=synthetic_targets()) +def test_conflict_check_matches_pairwise_definition(column_of, targets): + """The grouped check flags exactly the (column, year) pairs a pairwise + comparison does. Covers custom columns, counts against sums, country + restrictions and the nearest-earlier-year fallback.""" + grouped = {(key, year) for key, year, _ in conflicts(targets, column_of)} + assert grouped == _brute_force(targets, column_of)