diff --git a/changelog.d/582.fixed.md b/changelog.d/582.fixed.md new file mode 100644 index 00000000..5752b15c --- /dev/null +++ b/changelog.d/582.fixed.md @@ -0,0 +1 @@ +The UK Budget reform contract's shared-parameter check now catches list-valued parameters changed in place and changes to node or scale metadata or a scale's band count. diff --git a/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py b/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py index 4ca13fac..996c36f6 100644 --- a/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py +++ b/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py @@ -1348,28 +1348,52 @@ def _baseline_parameter_reader() -> Callable[[str, int], float]: return parameter_reader(reference_parameters()) -def _parameter_values(parameters) -> dict[str, list[tuple]]: - """Each parameter's dated values and metadata, by name. +def _frozen(value: Any) -> str: + """A value as text, so a list or dict changed in place still shows. + + JSON where it can be (key order ignored); otherwise ``repr``, so an + unusual value (non-string keys, a cycle) cannot stop the run. + """ + try: + return json.dumps(value, sort_keys=True, default=str) + except (TypeError, ValueError): + return repr(value) + - Every entry of its timeline (date and value) and its metadata, so a - change on any date, or to how it is uprated, shows. A name can occur - more than once (``parameters.baseline`` repeats the tree), so every - occurrence is kept, in tree order. +def _parameter_values(parameters) -> dict[str, list[tuple]]: + """Each parameter's dated values and metadata, and each node's metadata. + + For a parameter, every entry of its timeline (date and value) and its + metadata, so a change on any date, or to how it is uprated, shows; for + the root, every node and every scale, its metadata and, for a scale, + its bands' names in order, so an added, removed or reordered band + shows. Values are recorded as text, so a list changed in place is + caught. A name can occur more than once (``parameters.baseline`` + repeats the tree), so every occurrence is kept, in tree order. """ - from policyengine_core.parameters import Parameter + from policyengine_core.parameters import Parameter, ParameterScale values: dict[str, list[tuple]] = {} - for parameter in parameters.get_descendants(): - if isinstance(parameter, Parameter): - values.setdefault(parameter.name, []).append( + for item in [parameters, *parameters.get_descendants()]: + if isinstance(item, Parameter): + values.setdefault(item.name, []).append( ( tuple( - (entry.instant_str, entry.value) - for entry in parameter.values_list + (entry.instant_str, _frozen(entry.value)) + for entry in item.values_list ), - json.dumps(parameter.metadata, sort_keys=True, default=str), + _frozen(item.metadata), ) ) + else: + bands = ( + tuple(band.name for band in item.brackets) + if isinstance(item, ParameterScale) + else None + ) + values.setdefault(f"{item.name}#node", []).append( + (_frozen(getattr(item, "metadata", None)), bands) + ) return values @@ -1407,7 +1431,9 @@ def _state_values(tree) -> dict: if isinstance(tree, Mapping): # Timelines by name: each entry's date and value. return { - name: [(entry.instant_str, entry.value) for entry in timeline or []] + name: [ + (entry.instant_str, _frozen(entry.value)) for entry in timeline or [] + ] for name, timeline in tree.items() } return _parameter_values(tree) @@ -1440,8 +1466,8 @@ def module_parameters_unchanged(before: dict) -> dict: "route": "all", "reform_digest": None, "check": ( - "every parameter's dated values and metadata in " - f"{', '.join(sorted(before))}" + "every parameter's dated values and metadata, and every node's " + f"metadata and band order, in {', '.join(sorted(before))}" ), "status": FAIL if changed else PASS, "expected": "policyengine-uk's shared parameters as they were before the run", diff --git a/tests/test_uk_budget_reform_contract.py b/tests/test_uk_budget_reform_contract.py index d07e91c9..b70e2514 100644 --- a/tests/test_uk_budget_reform_contract.py +++ b/tests/test_uk_budget_reform_contract.py @@ -298,6 +298,75 @@ def test_module_parameters_unchanged_compares_the_shared_state(): assert row["status"] == "fail" and f"{tree}:{RATE}" in row["actual"] +def _fake_shared_tree(monkeypatch, data): + from policyengine_core.parameters import ParameterNode + + tree = ParameterNode("", data=data) + monkeypatch.setattr(contract, "_shared_trees", lambda: {"system": tree}) + return tree + + +def test_a_list_value_changed_in_place_is_caught(monkeypatch): + tree = _fake_shared_tree( + monkeypatch, {"sources": {"values": {"2000-01-01": ["earnings"]}}} + ) + before = contract.shared_state() + tree.sources.values_list[0].value.append("pensions") + row = contract.module_parameters_unchanged(before) + assert row["status"] == "fail" and "system:sources" in row["actual"] + + +def test_a_scale_metadata_or_band_change_is_caught(monkeypatch): + band = { + "threshold": {"values": {"2000-01-01": 0}}, + "amount": {"values": {"2000-01-01": 100}}, + } + tree = _fake_shared_tree( + monkeypatch, + {"scale": {"metadata": {"type": "single_amount"}, "brackets": [band]}}, + ) + before = contract.shared_state() + tree.scale.metadata["type"] = "marginal_amount" + row = contract.module_parameters_unchanged(before) + assert row["status"] == "fail" and "system:scale#node" in row["actual"] + tree.scale.metadata["type"] = "single_amount" + assert contract.module_parameters_unchanged(before)["status"] == "pass" + tree.scale.brackets.append(tree.scale.brackets[0].clone()) + assert contract.module_parameters_unchanged(before)["status"] == "fail" + + +def test_reordered_bands_and_root_metadata_are_caught(monkeypatch): + def band(rate): + return { + "threshold": {"values": {"2000-01-01": 0}}, + "rate": {"values": {"2000-01-01": rate}}, + } + + tree = _fake_shared_tree( + monkeypatch, {"scale": {"brackets": [band(0.1), band(0.2)]}} + ) + before = contract.shared_state() + tree.scale.brackets.reverse() + row = contract.module_parameters_unchanged(before) + assert row["status"] == "fail" and "system:scale#node" in row["actual"] + tree.scale.brackets.reverse() + assert contract.module_parameters_unchanged(before)["status"] == "pass" + tree.metadata["uprating"] = "wages" + row = contract.module_parameters_unchanged(before) + assert row["status"] == "fail" and "system:#node" in row["actual"] + + +def test_an_unusual_value_is_compared_without_stopping_the_run(monkeypatch): + import datetime as dt + + tree = _fake_shared_tree(monkeypatch, {"p": {"values": {"2000-01-01": 1}}}) + tree.p.values_list[0].value = {dt.date(2020, 1, 1): 1} + before = contract.shared_state() + assert contract.module_parameters_unchanged(before)["status"] == "pass" + tree.p.values_list[0].value[dt.date(2020, 1, 1)] = 2 + assert contract.module_parameters_unchanged(before)["status"] == "fail" + + def test_module_parameters_unchanged_compares_projected_timelines(): before = contract.shared_state() if "projected" not in before: