diff --git a/changelog.d/2412.removed.md b/changelog.d/2412.removed.md new file mode 100644 index 0000000000..a31f657fc6 --- /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. 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/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..a3d3db9000 --- /dev/null +++ b/policyengine_uk/tests/code_health/test_labour_supply_parameters_are_read.py @@ -0,0 +1,242 @@ +"""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) + + 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 + + +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(): + getitem = ParameterNodeAtInstant.__getitem__ + get_at_instant = AtInstantLike.get_at_instant + 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", + } + # 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 new file mode 100644 index 0000000000..b3a393afc5 --- /dev/null +++ b/policyengine_uk/tests/test_removed_obr_elasticities_parameter.py @@ -0,0 +1,237 @@ +"""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.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 + +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 + + +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) + + +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 fdbe9b663a..f7aab1efc3 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,45 @@ 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, called on a " + "simulation with a baseline, 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}" ) @@ -155,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 @@ -180,25 +212,34 @@ 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 + 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) 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