From c83e58488a40d1cb2ed6495f247cdcd3c2c5234b Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Wed, 14 Oct 2026 11:45:09 -0400 Subject: [PATCH 1/4] Remove the dead obr_elasticities labour supply switch gov.simulation.labour_supply_responses.obr_elasticities ("Use the OBR's progression elasticities for the labour supply responses") has had no reader since it was added in #993: setting it changed no output. The OBR elasticities are applied by Simulation.apply_dynamics under gov.dynamic.obr_labour_supply_assumptions, which this leaves unchanged. Add a code-health test that runs the elasticity-based response and the OBR dynamics on a small household, records every parameter read, and fails if any parameter under gov.simulation.labour_supply_responses or gov.dynamic was never read. A control parameter nothing reads must stay unread, so the recorder cannot pass by counting the whole tree. Fixes #2412 Co-Authored-By: Claude Opus 5.5 --- changelog.d/2412.removed.md | 1 + .../obr_elasticities.yaml | 6 - .../test_labour_supply_parameters_are_read.py | 238 ++++++++++++++++++ 3 files changed, 239 insertions(+), 6 deletions(-) create mode 100644 changelog.d/2412.removed.md delete mode 100644 policyengine_uk/parameters/gov/simulation/labour_supply_responses/obr_elasticities.yaml create mode 100644 policyengine_uk/tests/code_health/test_labour_supply_parameters_are_read.py diff --git a/changelog.d/2412.removed.md b/changelog.d/2412.removed.md new file mode 100644 index 0000000000..bab274dd6e --- /dev/null +++ b/changelog.d/2412.removed.md @@ -0,0 +1 @@ +- Removed `gov.simulation.labour_supply_responses.obr_elasticities` and its legacy alias `gov.simulation.labor_supply_responses.obr_elasticities`. Nothing read this switch, so setting it changed no output. The OBR labour supply elasticities are applied by `Simulation.apply_dynamics` when `gov.dynamic.obr_labour_supply_assumptions` is on. diff --git a/policyengine_uk/parameters/gov/simulation/labour_supply_responses/obr_elasticities.yaml b/policyengine_uk/parameters/gov/simulation/labour_supply_responses/obr_elasticities.yaml deleted file mode 100644 index 041dd41fc8..0000000000 --- a/policyengine_uk/parameters/gov/simulation/labour_supply_responses/obr_elasticities.yaml +++ /dev/null @@ -1,6 +0,0 @@ -description: Use the OBR's progression elasticities for the labour supply responses. -values: - 2000-01-01: 0 -metadata: - unit: currency-GBP - reference: none \ No newline at end of file diff --git a/policyengine_uk/tests/code_health/test_labour_supply_parameters_are_read.py b/policyengine_uk/tests/code_health/test_labour_supply_parameters_are_read.py new file mode 100644 index 0000000000..b8414660a7 --- /dev/null +++ b/policyengine_uk/tests/code_health/test_labour_supply_parameters_are_read.py @@ -0,0 +1,238 @@ +"""Every labour supply parameter must be read by a formula or by the dynamics. + +``gov.simulation.labour_supply_responses.obr_elasticities`` ("Use the OBR's +progression elasticities for the labour supply responses") stayed in the tree +from October 2024 to October 2026 although nothing read it, so setting it +changed no output (#2412). The OBR elasticities are applied by +``Simulation.apply_dynamics`` when ``gov.dynamic.obr_labour_supply_assumptions`` +is on. + +This test runs both labour supply response paths on a small household and +records every parameter read along the way: + +- the elasticity-based response (``employment_income_behavioral_response``) + with non-zero elasticities, so that it gets past its early return and reads + the bounds too; +- the OBR dynamics (``Simulation.apply_dynamics``) with the OBR assumptions on. + +It fails if a parameter under ``GUARDED_PREFIXES`` was never read. A new +parameter there needs a reader. If it is read only under a condition this +scenario does not meet, extend the scenario. + +Reads are recorded on the parameter tree itself, so a read counts whether a +formula makes it through ``parameters(period)`` or the dynamics make it through +``sim.tax_benefit_system.parameters``. Three kinds of read are recorded: an +attribute or item of a node at an instant that holds a value (not a node), a +direct call of a parameter or scale (``parameter(year)``), and fancy indexing +of a node at an instant (``node[array]``), which reads every leaf under that +node. Building a node at an instant reads nothing: it resolves every child, +whether or not anything uses them. + +A control parameter that nothing reads is added to the tree of both runs. It +must stay unread, which shows the recorder does not count a parameter just +because it is in the tree. +""" + +import contextlib + +import numpy as np +import pytest +from policyengine_core.parameters import ( + Parameter, + ParameterNode, + ParameterNodeAtInstant, + ParameterScale, +) +from policyengine_core.parameters.at_instant_like import AtInstantLike + +from policyengine_uk import CountryTaxBenefitSystem, Microsimulation, Simulation +from policyengine_uk.model_api import Scenario +from policyengine_uk.parameters.gov.simulation.labour_supply_responses.aliases import ( + canonicalize_lsr_parameter_path, +) + +GUARDED_PREFIXES = ( + "gov.simulation.labour_supply_responses", + "gov.dynamic", +) +YEAR = 2025 +BASIC_RATE_RISE = {"gov.hmrc.income_tax.rates.uk[0].rate": {str(YEAR): 0.30}} +ELASTICITIES = { + "gov.simulation.labour_supply_responses.income_elasticity": {str(YEAR): -0.05}, + "gov.simulation.labour_supply_responses.substitution_elasticity": {str(YEAR): 0.25}, +} +OBR_ASSUMPTIONS = {"gov.dynamic.obr_labour_supply_assumptions": {str(YEAR): True}} +CONTROL_NODE = "gov.simulation.labour_supply_responses" +CONTROL = f"{CONTROL_NODE}.unread_control" + + +def leaf_paths(node: ParameterNode, path: str) -> set: + """The path of every parameter and scale under ``node``.""" + leaves = set() + for key, child in node.children.items(): + child_path = f"{path}.{key}" + if isinstance(child, ParameterNode): + leaves |= leaf_paths(child, child_path) + else: + leaves.add(child_path) + return leaves + + +def _leaves_at_instant(node: ParameterNodeAtInstant, path: str): + for key, child in object.__getattribute__(node, "_children").items(): + if isinstance(child, ParameterNodeAtInstant): + yield from _leaves_at_instant(child, f"{path}.{key}") + else: + yield f"{path}.{key}" + + +@contextlib.contextmanager +def record_parameter_reads(): + """Record the path of every parameter read in the block.""" + reads = set() + getattribute = ParameterNodeAtInstant.__getattribute__ + getitem = ParameterNodeAtInstant.__getitem__ + get_at_instant = AtInstantLike.get_at_instant + + def recording_getattribute(self, key): + value = getattribute(self, key) + if ( + not key.startswith("_") + and not isinstance(value, ParameterNodeAtInstant) + and key in getattribute(self, "_children") + ): + reads.add(f"{getattribute(self, '_name')}.{key}") + return value + + def recording_getitem(self, key): + value = getitem(self, key) + name = getattribute(self, "_name") + if not isinstance(key, str): + # Fancy indexing: the vectorial node reads the whole subtree. + reads.update(_leaves_at_instant(self, name)) + elif not isinstance(value, ParameterNodeAtInstant): + reads.add(f"{name}.{key}") + return value + + def recording_get_at_instant(self, instant): + if isinstance(self, (Parameter, ParameterScale)): + reads.add(self.name) + return get_at_instant(self, instant) + + ParameterNodeAtInstant.__getattribute__ = recording_getattribute + ParameterNodeAtInstant.__getitem__ = recording_getitem + AtInstantLike.get_at_instant = recording_get_at_instant + try: + yield reads + finally: + ParameterNodeAtInstant.__getattribute__ = getattribute + ParameterNodeAtInstant.__getitem__ = getitem + AtInstantLike.get_at_instant = get_at_instant + + +def _couple_with_child() -> dict: + members = ["adult_1", "adult_2", "child"] + return { + "people": { + "adult_1": {"age": 45, "employment_income": 60_000, "hours_worked": 1_800}, + "adult_2": {"age": 43, "employment_income": 18_000, "hours_worked": 1_000}, + "child": {"age": 7}, + }, + "benunits": {"benunit": {"members": members}}, + "households": {"household": {"members": members}}, + } + + +def _add_unread_control(simulation) -> None: + node = simulation.tax_benefit_system.parameters.get_child(CONTROL_NODE) + node.add_child( + "unread_control", + Parameter( + CONTROL, + { + "description": "A parameter nothing reads.", + "values": {"2000-01-01": 1}, + }, + ), + ) + simulation.tax_benefit_system.reset_parameter_caches() + + +@pytest.fixture(scope="module") +def labour_supply_run(): + """Run both labour supply response paths and record the parameters read.""" + formula_sim = Simulation( + situation=_couple_with_child(), + scenario=Scenario(parameter_changes={**ELASTICITIES, **BASIC_RATE_RISE}), + ) + dynamics_sim = Microsimulation( + situation=_couple_with_child(), + scenario=Scenario(parameter_changes={**OBR_ASSUMPTIONS, **BASIC_RATE_RISE}), + ) + dynamics_sim.baseline = Microsimulation(situation=_couple_with_child()) + simulations = (formula_sim, dynamics_sim) + for simulation in simulations: + _add_unread_control(simulation) + + with record_parameter_reads() as reads: + formula_sim.calculate("employment_income_behavioral_response", YEAR) + dynamics = dynamics_sim.apply_dynamics(YEAR) + + # Both paths ran in full: the formula built its measurement branch, which + # it does only after its early returns, and the dynamics returned results. + assert "lsr_measurement" in formula_sim.branches + assert dynamics is not None + return simulations, {canonicalize_lsr_parameter_path(read) for read in reads} + + +@pytest.mark.parametrize("prefix", GUARDED_PREFIXES) +def test_every_labour_supply_parameter_is_read(prefix, labour_supply_run): + _, reads = labour_supply_run + parameters = CountryTaxBenefitSystem().parameters + leaves = leaf_paths(parameters.get_child(prefix), prefix) + assert leaves, f"No parameters found under {prefix}." + + unread = sorted(leaves - reads) + assert not unread, ( + f"No formula or dynamics read these parameters under {prefix}: " + f"{unread}. Setting them changes no output. Delete them or read them; " + "if they are read only under a condition, extend the scenario in " + "this test to meet it." + ) + + +def test_recorder_does_not_count_a_parameter_nothing_reads(labour_supply_run): + simulations, reads = labour_supply_run + for simulation in simulations: + # The control was in the tree for the whole run. + assert simulation.tax_benefit_system.parameters.get_child(CONTROL) + assert CONTROL not in reads + + +def test_recorder_counts_each_kind_of_read(): + leaf = {"values": {"2000-01-01": 1}} + root = ParameterNode( + "root", + data={ + "by_attribute": leaf, + "by_item": leaf, + "by_call": leaf, + "unread": leaf, + "vector": {"a": leaf, "b": leaf}, + }, + ) + with record_parameter_reads() as reads: + at_instant = root("2020-01-01") + assert reads == set(), "Building a node at an instant is not a read." + at_instant.by_attribute + at_instant["by_item"] + root.by_call("2020-01-01") + at_instant.vector[np.array(["a", "a"])] + + assert reads == { + "root.by_attribute", + "root.by_item", + "root.by_call", + "root.vector.a", + "root.vector.b", + } From d9706642fbbf8d247cc04426a70dab9cc35dc154 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Wed, 14 Oct 2026 11:50:50 -0400 Subject: [PATCH 2/4] Give directions when a reform names the removed OBR elasticities switch Register gov.simulation.labour_supply_responses.obr_elasticities in REMOVED_PARAMETERS, the repo's registry for removed parameters, so a reform or saved policy that still names it (the app-v2 parameter tree listed it) fails with a message pointing to gov.dynamic.obr_labour_supply_assumptions instead of a bare lookup error. The stub is hidden from the app (economy and household false). add_removed_parameter_aliases was hard-wired to the State Pension age node; it now places each stub under its own parent, and each entry carries its own label. check_parameter_not_removed checks a legacy labor_supply_responses path as its labour_supply_responses equivalent, so the legacy alias gives the same directions at every entry point. Co-Authored-By: Claude Opus 5.5 --- changelog.d/2412.removed.md | 2 +- ...test_removed_obr_elasticities_parameter.py | 187 ++++++++++++++++++ policyengine_uk/utils/parameters.py | 67 +++++-- 3 files changed, 238 insertions(+), 18 deletions(-) create mode 100644 policyengine_uk/tests/test_removed_obr_elasticities_parameter.py diff --git a/changelog.d/2412.removed.md b/changelog.d/2412.removed.md index bab274dd6e..a31f657fc6 100644 --- a/changelog.d/2412.removed.md +++ b/changelog.d/2412.removed.md @@ -1 +1 @@ -- Removed `gov.simulation.labour_supply_responses.obr_elasticities` and its legacy alias `gov.simulation.labor_supply_responses.obr_elasticities`. Nothing read this switch, so setting it changed no output. The OBR labour supply elasticities are applied by `Simulation.apply_dynamics` when `gov.dynamic.obr_labour_supply_assumptions` is on. +- Removed `gov.simulation.labour_supply_responses.obr_elasticities` and its legacy alias `gov.simulation.labor_supply_responses.obr_elasticities`. Nothing read this switch, so setting it changed no output. A reform or saved policy that still names it now fails with directions to `gov.dynamic.obr_labour_supply_assumptions`, the switch under which `Simulation.apply_dynamics` applies the OBR labour supply elasticities. diff --git a/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py b/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py new file mode 100644 index 0000000000..6dff1622f7 --- /dev/null +++ b/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py @@ -0,0 +1,187 @@ +"""The removed OBR elasticities switch gives directions at every reform entry. + +``gov.simulation.labour_supply_responses.obr_elasticities`` was removed because +nothing read it (#2412). A reform or saved policy that still names it, under +the current path or the legacy ``labor_supply_responses`` one, fails with a +message pointing to ``gov.dynamic.obr_labour_supply_assumptions`` rather than +a bare lookup error. +""" + +import copy +from types import SimpleNamespace + +import pytest +from policyengine_core.parameters import ParameterNode, get_parameter +from policyengine_core.reforms import Reform, set_parameter + +from policyengine_uk import Simulation +from policyengine_uk.system import system +from policyengine_uk.utils.parameters import RemovedParameterNode +from policyengine_uk.utils.scenario import Scenario + +CANONICAL = "gov.simulation.labour_supply_responses.obr_elasticities" +LEGACY = "gov.simulation.labor_supply_responses.obr_elasticities" +REMOVED_PATHS = [CANONICAL, LEGACY] +SITUATION = { + "people": {"person": {"age": 30, "employment_income": 25_000}}, + "benunits": {"benunit": {"members": ["person"]}}, + "households": {"household": {"members": ["person"]}}, +} + + +@pytest.fixture(scope="module", autouse=True) +def reuse_country_model(): + # Build each simulation's system by cloning the loaded one rather than + # loading the whole country model for every case. + with pytest.MonkeyPatch.context() as patch: + patch.setattr( + "policyengine_uk.simulation.CountryTaxBenefitSystem", system.clone + ) + yield + + +@pytest.fixture(scope="module") +def baseline_simulation(): + return Simulation(situation=SITUATION) + + +@pytest.fixture +def simulation(baseline_simulation): + return baseline_simulation.clone() + + +def assert_directions(message: str) -> None: + assert "supply_responses.obr_elasticities has been removed" in message + assert "Nothing read it" in message + assert "gov.dynamic.obr_labour_supply_assumptions" in message + + +ENTRY_POINTS = [ + "parameter_lookup", + "simulation_parameter_changes", + "scenario_dict_modifier", + "scenario_apply_scalar", + "scenario_apply_periods", + "simulation_apply_dict", + "system_apply_dict", + "system_modify_dict", + "core_reform_dict", + "core_set_parameter", + "core_reform_api", + "api_parameter_normalization", + "api_legacy_policy_normalization", +] + + +@pytest.mark.parametrize("path", REMOVED_PATHS) +@pytest.mark.parametrize("entry_point", ENTRY_POINTS) +def test_removed_switch_entry_points_give_directions( + simulation, monkeypatch, path, entry_point +): + changes = {path: {"2025": 1}} + tax_benefit_system = simulation.tax_benefit_system + + with pytest.raises(ValueError) as error: + if entry_point == "parameter_lookup": + tax_benefit_system.parameters.get_child(path) + elif entry_point == "simulation_parameter_changes": + simulation.apply_parameter_changes(changes) + elif entry_point == "scenario_dict_modifier": + Scenario.from_reform(changes).simulation_modifier(simulation) + elif entry_point == "scenario_apply_scalar": + Scenario(parameter_changes={path: 1}).apply(simulation) + elif entry_point == "scenario_apply_periods": + Scenario(parameter_changes=changes).apply(simulation) + elif entry_point == "simulation_apply_dict": + simulation.apply_reform(changes) + elif entry_point == "system_apply_dict": + tax_benefit_system.apply_reform_set(changes) + elif entry_point == "system_modify_dict": + tax_benefit_system.modify_parameters(changes) + elif entry_point == "core_reform_dict": + Reform.from_dict(changes)(tax_benefit_system) + elif entry_point == "core_set_parameter": + set_parameter(path, 1, period="2025")(tax_benefit_system) + elif entry_point == "core_reform_api": + response = SimpleNamespace( + json=lambda: { + "result": { + "policy_json": {path: {"2025-01-01.2025-12-31": 1}}, + "label": "saved OBR elasticities reform", + } + } + ) + monkeypatch.setattr( + "policyengine_core.reforms.reform.requests.get", + lambda *a, **k: response, + ) + Reform.from_api("obr-elasticities", country_id="uk")(tax_benefit_system) + elif entry_point == "api_parameter_normalization": + # policyengine-api reads the final value to infer a type before it + # calls Parameter.update. + get_parameter(tax_benefit_system.parameters, path).values_list[-1].value + elif entry_point == "api_legacy_policy_normalization": + # It also walks the children, then reads values_list. + node = tax_benefit_system.parameters + for part in path.split("."): + node = node.children[part] + node.values_list[-1].value + else: + raise AssertionError(f"Unknown entry point: {entry_point}") + + assert_directions(str(error.value)) + + +@pytest.mark.parametrize("path", REMOVED_PATHS) +@pytest.mark.parametrize("entry_point", ["reform_dict", "scenario"]) +def test_simulation_constructor_rejects_removed_switch(path, entry_point): + changes = {path: {"2025": 1}} + if entry_point == "reform_dict": + kwargs = {"reform": changes} + else: + kwargs = {"scenario": Scenario(parameter_changes=changes)} + + with pytest.raises(ValueError) as error: + Simulation(situation=SITUATION, **kwargs) + assert_directions(str(error.value)) + + +@pytest.mark.parametrize("path", REMOVED_PATHS) +@pytest.mark.parametrize("clone_method", ["clone", "deepcopy"]) +def test_removed_switch_stub_is_hidden_and_survives_copies( + baseline_simulation, path, clone_method +): + original = baseline_simulation.tax_benefit_system.parameters + parameters = ( + original.clone() if clone_method == "clone" else copy.deepcopy(original) + ) + stub = parameters + for part in path.split("."): + stub = stub.children[part] + assert isinstance(stub, RemovedParameterNode) + assert isinstance(stub, ParameterNode) + assert not stub.children + assert stub.metadata["removed"] is True + assert stub.metadata["economy"] is False + assert stub.metadata["household"] is False + assert stub.metadata["label"] == "removed OBR elasticities switch" + assert_directions(stub.description) + with pytest.raises(ValueError) as error: + parameters.get_child(path) + assert_directions(str(error.value)) + with pytest.raises(ValueError) as error: + stub.update(period="2025", value=1) + assert_directions(str(error.value)) + + +@pytest.mark.parametrize( + "path", + [ + "gov.simulation.labour_supply_responses.income_elasticity", + "gov.simulation.labor_supply_responses.income_elasticity", + ], +) +def test_neighbouring_labour_supply_parameters_can_still_be_reformed(simulation, path): + simulation.apply_parameter_changes({path: {"2025": -0.05}}) + lsr = simulation.tax_benefit_system.parameters.gov.simulation + assert lsr.labour_supply_responses.income_elasticity("2025") == -0.05 diff --git a/policyengine_uk/utils/parameters.py b/policyengine_uk/utils/parameters.py index fdbe9b663a..c25662bce3 100644 --- a/policyengine_uk/utils/parameters.py +++ b/policyengine_uk/utils/parameters.py @@ -1,8 +1,13 @@ import datetime +from typing import NamedTuple from policyengine_core.model_api import * from policyengine_core import periods +from policyengine_uk.parameters.gov.simulation.labour_supply_responses.aliases import ( + canonicalize_lsr_parameter_path, +) + def str_to_instant(s): return periods.Instant(tuple(map(lambda s: int(s), s.split("-")))) @@ -132,20 +137,44 @@ def convert_to_fiscal_year_parameters(parameters): "variables." ) -# Parameters removed from the tree, with what replaces them, so a reform or -# saved policy that still names one fails with directions instead of a bare -# lookup error. +_OBR_ELASTICITIES_REPLACEMENT = ( + "Nothing read it, so setting it never changed any output. The OBR's labour " + "supply elasticities are applied by Simulation.apply_dynamics when " + "gov.dynamic.obr_labour_supply_assumptions is true, which it is by default." +) + + +class RemovedParameter(NamedTuple): + label: str + replacement: str + + +# Parameters removed from the tree, with the label of the node left at each +# path and what replaces the parameter, so a reform or saved policy that still +# names one fails with directions instead of a bare lookup error. REMOVED_PARAMETERS = { - "gov.dwp.state_pension.age.male": _STATE_PENSION_AGE_REPLACEMENT, - "gov.dwp.state_pension.age.female": _STATE_PENSION_AGE_REPLACEMENT, + "gov.dwp.state_pension.age.male": RemovedParameter( + "removed male State Pension age parameter", _STATE_PENSION_AGE_REPLACEMENT + ), + "gov.dwp.state_pension.age.female": RemovedParameter( + "removed female State Pension age parameter", _STATE_PENSION_AGE_REPLACEMENT + ), + "gov.simulation.labour_supply_responses.obr_elasticities": RemovedParameter( + "removed OBR elasticities switch", _OBR_ELASTICITIES_REPLACEMENT + ), } def check_parameter_not_removed(path: str) -> None: - """Raise a ValueError naming the replacement if path was removed.""" - if path in REMOVED_PARAMETERS: + """Raise a ValueError naming the replacement if path was removed. + + A legacy ``labor_supply_responses`` path is checked as its + ``labour_supply_responses`` equivalent. + """ + removed = REMOVED_PARAMETERS.get(canonicalize_lsr_parameter_path(path)) + if removed is not None: raise ValueError( - f"The parameter {path} has been removed. {REMOVED_PARAMETERS[path]}" + f"The parameter {path} has been removed. {removed.replacement}" ) @@ -180,10 +209,12 @@ def add_removed_parameter_aliases(parameters: ParameterNode) -> ParameterNode: for child in root.children.values(): child.parent = root parameters = root - age = parameters.gov.dwp.state_pension.age - for path, replacement in REMOVED_PARAMETERS.items(): - name = path.rsplit(".", 1)[1] - previous = age.children.get(name) + for path, removed in REMOVED_PARAMETERS.items(): + parent_path, name = path.rsplit(".", 1) + parent = parameters + for part in parent_path.split("."): + parent = parent.children[part] + previous = parent.children.get(name) if isinstance(previous, RemovedParameterNode): continue alias = RemovedParameterNode(path, data={}) @@ -191,14 +222,16 @@ def add_removed_parameter_aliases(parameters: ParameterNode) -> ParameterNode: alias.metadata.update(previous.metadata) for child_name, child in previous.children.items(): alias.add_child(child_name, child) - alias.description = f"The parameter {path} has been removed. {replacement}" + alias.description = ( + f"The parameter {path} has been removed. {removed.replacement}" + ) alias.metadata.update( - label=f"removed {name} State Pension age parameter", + label=removed.label, removed=True, economy=False, household=False, ) - age.children[name] = alias - setattr(age, name, alias) - alias.parent = age + parent.children[name] = alias + setattr(parent, name, alias) + alias.parent = parent return parameters From 76f6aba56199ad6921d6a9cd45f3126618bd3a9c Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Wed, 14 Oct 2026 11:57:40 -0400 Subject: [PATCH 3/4] Test that the registry rejects exactly the removed parameter paths Walk every path under gov, the legacy labor_supply_responses alias included, and check that check_parameter_not_removed rejects a path exactly when a removed-parameter stub sits there, and that every registered path has a stub. Co-Authored-By: Claude Opus 5.5 --- ...test_removed_obr_elasticities_parameter.py | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py b/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py index 6dff1622f7..3501d1dcb7 100644 --- a/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py +++ b/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py @@ -16,7 +16,14 @@ from policyengine_uk import Simulation from policyengine_uk.system import system -from policyengine_uk.utils.parameters import RemovedParameterNode +from policyengine_uk.parameters.gov.simulation.labour_supply_responses.aliases import ( + canonicalize_lsr_parameter_path, +) +from policyengine_uk.utils.parameters import ( + REMOVED_PARAMETERS, + RemovedParameterNode, + check_parameter_not_removed, +) from policyengine_uk.utils.scenario import Scenario CANONICAL = "gov.simulation.labour_supply_responses.obr_elasticities" @@ -185,3 +192,30 @@ def test_neighbouring_labour_supply_parameters_can_still_be_reformed(simulation, simulation.apply_parameter_changes({path: {"2025": -0.05}}) lsr = simulation.tax_benefit_system.parameters.gov.simulation assert lsr.labour_supply_responses.income_elasticity("2025") == -0.05 + + +def _node_paths(node: ParameterNode, path: str): + for key, child in node.children.items(): + child_path = f"{path}.{key}" + yield child_path, child + if isinstance(child, ParameterNode): + yield from _node_paths(child, child_path) + + +def test_registry_rejects_exactly_the_removed_paths(): + # Every path under gov, legacy aliases included: the check rejects a path + # exactly when a removed-parameter stub sits there, and every registered + # path has a stub. (The tree's internal ``baseline`` copy is not a path + # reforms name.) + stubs = set() + for path, node in _node_paths(system.parameters.gov, "gov"): + is_stub = isinstance(node, RemovedParameterNode) + try: + check_parameter_not_removed(path) + rejected = False + except ValueError: + rejected = True + assert rejected == is_stub, path + if is_stub: + stubs.add(canonicalize_lsr_parameter_path(path)) + assert stubs == set(REMOVED_PARAMETERS) From aca90a49c1dd8994465162a23cf9abe9c6022617 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Wed, 14 Oct 2026 15:15:55 -0400 Subject: [PATCH 4/4] Address review: clear error for a registered path that still exists - add_removed_parameter_aliases raises a ValueError telling you to delete the YAML when a path in REMOVED_PARAMETERS still holds a parameter, instead of an AttributeError at import. - Note on RemovedParameterNode that it reads as an empty node at an instant, so its parent can still be read. - The OBR elasticities message says apply_dynamics acts on a simulation with a baseline. - The read recorder patches the classes with MonkeyPatch.context(), so it leaves no attribute behind; the recorder test checks that. Co-Authored-By: Claude Opus 5.5 --- .../test_labour_supply_parameters_are_read.py | 20 +++++++++++-------- ...test_removed_obr_elasticities_parameter.py | 16 +++++++++++++++ policyengine_uk/utils/parameters.py | 12 +++++++++-- 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/policyengine_uk/tests/code_health/test_labour_supply_parameters_are_read.py b/policyengine_uk/tests/code_health/test_labour_supply_parameters_are_read.py index b8414660a7..a3d3db9000 100644 --- a/policyengine_uk/tests/code_health/test_labour_supply_parameters_are_read.py +++ b/policyengine_uk/tests/code_health/test_labour_supply_parameters_are_read.py @@ -119,15 +119,13 @@ def recording_get_at_instant(self, instant): reads.add(self.name) return get_at_instant(self, instant) - ParameterNodeAtInstant.__getattribute__ = recording_getattribute - ParameterNodeAtInstant.__getitem__ = recording_getitem - AtInstantLike.get_at_instant = recording_get_at_instant - try: + with pytest.MonkeyPatch.context() as patch: + patch.setattr( + ParameterNodeAtInstant, "__getattribute__", recording_getattribute + ) + patch.setattr(ParameterNodeAtInstant, "__getitem__", recording_getitem) + patch.setattr(AtInstantLike, "get_at_instant", recording_get_at_instant) yield reads - finally: - ParameterNodeAtInstant.__getattribute__ = getattribute - ParameterNodeAtInstant.__getitem__ = getitem - AtInstantLike.get_at_instant = get_at_instant def _couple_with_child() -> dict: @@ -210,6 +208,8 @@ def test_recorder_does_not_count_a_parameter_nothing_reads(labour_supply_run): def test_recorder_counts_each_kind_of_read(): + getitem = ParameterNodeAtInstant.__getitem__ + get_at_instant = AtInstantLike.get_at_instant leaf = {"values": {"2000-01-01": 1}} root = ParameterNode( "root", @@ -236,3 +236,7 @@ def test_recorder_counts_each_kind_of_read(): "root.vector.a", "root.vector.b", } + # The recorder leaves the classes as it found them. + assert "__getattribute__" not in ParameterNodeAtInstant.__dict__ + assert ParameterNodeAtInstant.__getitem__ is getitem + assert AtInstantLike.get_at_instant is get_at_instant diff --git a/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py b/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py index 3501d1dcb7..b3a393afc5 100644 --- a/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py +++ b/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py @@ -19,9 +19,12 @@ from policyengine_uk.parameters.gov.simulation.labour_supply_responses.aliases import ( canonicalize_lsr_parameter_path, ) +from policyengine_uk.utils import parameters as parameter_utils from policyengine_uk.utils.parameters import ( REMOVED_PARAMETERS, + RemovedParameter, RemovedParameterNode, + add_removed_parameter_aliases, check_parameter_not_removed, ) from policyengine_uk.utils.scenario import Scenario @@ -219,3 +222,16 @@ def test_registry_rejects_exactly_the_removed_paths(): if is_stub: stubs.add(canonicalize_lsr_parameter_path(path)) assert stubs == set(REMOVED_PARAMETERS) + + +def test_registering_a_parameter_that_still_exists_fails_clearly(monkeypatch): + # Listing a parameter as removed without deleting its YAML must say so, + # not fail at import with an AttributeError. + monkeypatch.setattr( + parameter_utils, + "REMOVED_PARAMETERS", + {"gov.switch": RemovedParameter("removed switch", "Use another one.")}, + ) + tree = ParameterNode("", data={"gov": {"switch": {"values": {"2000-01-01": 1}}}}) + with pytest.raises(ValueError, match="Delete its YAML file"): + add_removed_parameter_aliases(tree) diff --git a/policyengine_uk/utils/parameters.py b/policyengine_uk/utils/parameters.py index c25662bce3..f7aab1efc3 100644 --- a/policyengine_uk/utils/parameters.py +++ b/policyengine_uk/utils/parameters.py @@ -139,8 +139,9 @@ def convert_to_fiscal_year_parameters(parameters): _OBR_ELASTICITIES_REPLACEMENT = ( "Nothing read it, so setting it never changed any output. The OBR's labour " - "supply elasticities are applied by Simulation.apply_dynamics when " - "gov.dynamic.obr_labour_supply_assumptions is true, which it is by default." + "supply elasticities are applied by Simulation.apply_dynamics, called on a " + "simulation with a baseline, when gov.dynamic.obr_labour_supply_assumptions " + "is true, which it is by default." ) @@ -184,6 +185,8 @@ class RemovedParameterNode(ParameterNode): Male State Pension age still has valid children, so the compatibility node must support normal tree traversal and cloning. API reform code reads values_list before updating; both operations give migration help. + Read at an instant, the node is an empty node rather than an error, so + that its parent can still be read at an instant. """ @property @@ -217,6 +220,11 @@ def add_removed_parameter_aliases(parameters: ParameterNode) -> ParameterNode: previous = parent.children.get(name) if isinstance(previous, RemovedParameterNode): continue + if previous is not None and not isinstance(previous, ParameterNode): + raise ValueError( + f"{path} is listed in REMOVED_PARAMETERS but the parameter still " + "exists. Delete its YAML file, or remove it from REMOVED_PARAMETERS." + ) alias = RemovedParameterNode(path, data={}) if previous is not None: alias.metadata.update(previous.metadata)