From 878741dc46b5f788889685d2e229f75c8e3f041a Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 24 Oct 2026 07:33:00 -0400 Subject: [PATCH 1/6] Share nested metadata values between parameter clones Parameter, ParameterAtInstant, ParameterNode and ParameterScale clones deep-copied their metadata. On policyengine-us one tree clone makes 1,257,541 ParameterAtInstant clones, and those deep copies were most of the cost of TaxBenefitSystem.clone(). Each clone now gets its own metadata dict (copy.copy, so None and dict subclasses behave as before) holding the source's values. Top-level edits stay on their side; a nested value must be replaced, not edited in place. interpolate_parameters was the only writer that edited a nested value after a clone exists (TaxBenefitSystem.__init__ clones the tree into parameters.baseline before interpolating), so it now replaces the interpolation dict it marks complete. Co-Authored-By: Claude Opus 5.5 --- ...are-parameter-metadata-on-clone.changed.md | 1 + docs/usage/parameters.md | 10 + .../operations/interpolate_parameters.py | 7 +- policyengine_core/parameters/parameter.py | 3 +- .../parameters/parameter_at_instant.py | 4 +- .../parameters/parameter_node.py | 3 +- .../parameters/parameter_scale.py | 3 +- .../test_parameter_metadata_clone.py | 248 ++++++++++++++++++ .../test_parameter_metadata_clone_property.py | 148 +++++++++++ tests/fixtures/parameter_metadata_clones.py | 200 ++++++++++++++ 10 files changed, 622 insertions(+), 5 deletions(-) create mode 100644 changelog.d/share-parameter-metadata-on-clone.changed.md create mode 100644 tests/core/parameters/test_parameter_metadata_clone.py create mode 100644 tests/core/parameters/test_parameter_metadata_clone_property.py create mode 100644 tests/fixtures/parameter_metadata_clones.py diff --git a/changelog.d/share-parameter-metadata-on-clone.changed.md b/changelog.d/share-parameter-metadata-on-clone.changed.md new file mode 100644 index 000000000..c468da864 --- /dev/null +++ b/changelog.d/share-parameter-metadata-on-clone.changed.md @@ -0,0 +1 @@ +Parameter clones copy each metadata dict without deep-copying the values in it, which roughly halves the time to clone a large tax-benefit system; `interpolate_parameters` now replaces the `interpolation` dict it marks complete instead of editing it. diff --git a/docs/usage/parameters.md b/docs/usage/parameters.md index a7a257002..e876129ac 100644 --- a/docs/usage/parameters.md +++ b/docs/usage/parameters.md @@ -57,6 +57,16 @@ metadata: href: https://www.gov.uk/government/publications/child-benefit-rates-and-thresholds/child-benefit-rates-and-thresholds ``` +### Metadata in copies + +`clone()` on a parameter tree, which `TaxBenefitSystem.clone()`, reforms and simulation clones use, gives every node, parameter, dated value and scale in the copy its own metadata dict. The values in that dict are the same objects as in the source. Setting, updating or removing a key changes only the tree it is made on. To change a nested value, such as one entry of a `reference` list, replace the key with a new value instead of editing the list or dict in place, which would change every copy that shares it: + +```python +parameter.metadata["reference"] = [*parameter.metadata["reference"], new_reference] +``` + +The same holds within one tree: a parameter's dated values hold the parameter's metadata values, and `propagate_parameter_metadata` gives a node's descendants the node's values. + ### Metadata for parameters ```{eval-rst} diff --git a/policyengine_core/parameters/operations/interpolate_parameters.py b/policyengine_core/parameters/operations/interpolate_parameters.py index c91a8ce2f..6b4a0ef29 100644 --- a/policyengine_core/parameters/operations/interpolate_parameters.py +++ b/policyengine_core/parameters/operations/interpolate_parameters.py @@ -56,5 +56,10 @@ def interpolate_parameters(root: ParameterNode) -> ParameterNode: for entry in interpolated_entries: parameter.values_list.append(entry) parameter.values_list.sort(key=lambda x: x.instant_str, reverse=True) - parameter.metadata["interpolation"]["completed"] = True + # Replace the dict rather than edit it: a clone of this + # parameter shares it (see ParameterAtInstant.clone). + parameter.metadata["interpolation"] = { + **parameter.metadata["interpolation"], + "completed": True, + } return root diff --git a/policyengine_core/parameters/parameter.py b/policyengine_core/parameters/parameter.py index 50c3205f7..9bc5eb27d 100644 --- a/policyengine_core/parameters/parameter.py +++ b/policyengine_core/parameters/parameter.py @@ -124,7 +124,8 @@ def clone(self): clone = empty_clone(self) clone.__dict__ = self.__dict__.copy() - clone.metadata = copy.deepcopy(self.metadata) + # Own dict, shared values: see ParameterAtInstant.clone. + clone.metadata = copy.copy(self.metadata) clone.values_list = [ parameter_at_instant.clone() for parameter_at_instant in self.values_list ] diff --git a/policyengine_core/parameters/parameter_at_instant.py b/policyengine_core/parameters/parameter_at_instant.py index b815f1c10..1c2397713 100644 --- a/policyengine_core/parameters/parameter_at_instant.py +++ b/policyengine_core/parameters/parameter_at_instant.py @@ -76,5 +76,7 @@ def __repr__(self) -> str: def clone(self) -> "ParameterAtInstant": clone = commons.empty_clone(self) clone.__dict__ = self.__dict__.copy() - clone.metadata = copy.deepcopy(self.metadata) + # The clone gets its own metadata dict, but shares the values in it + # with this one. To change a nested value, replace its key. + clone.metadata = copy.copy(self.metadata) return clone diff --git a/policyengine_core/parameters/parameter_node.py b/policyengine_core/parameters/parameter_node.py index 0df0e387b..8338716f2 100644 --- a/policyengine_core/parameters/parameter_node.py +++ b/policyengine_core/parameters/parameter_node.py @@ -204,7 +204,8 @@ def clone(self) -> "ParameterNode": clone = commons.empty_clone(self) clone.__dict__ = self.__dict__.copy() - clone.metadata = copy.deepcopy(self.metadata) + # Own dict, shared values: see ParameterAtInstant.clone. + clone.metadata = copy.copy(self.metadata) clone.children = {key: child.clone() for key, child in self.children.items()} for child_key, child in clone.children.items(): setattr(clone, child_key, child) diff --git a/policyengine_core/parameters/parameter_scale.py b/policyengine_core/parameters/parameter_scale.py index 9b92a75b8..9c501765a 100644 --- a/policyengine_core/parameters/parameter_scale.py +++ b/policyengine_core/parameters/parameter_scale.py @@ -122,7 +122,8 @@ def clone(self) -> "ParameterScale": clone.brackets = [bracket.clone() for bracket in self.brackets] for bracket in clone.brackets: bracket.parent = clone - clone.metadata = copy.deepcopy(self.metadata) + # Own dict, shared values: see ParameterAtInstant.clone. + clone.metadata = copy.copy(self.metadata) return clone diff --git a/tests/core/parameters/test_parameter_metadata_clone.py b/tests/core/parameters/test_parameter_metadata_clone.py new file mode 100644 index 000000000..eed9595c3 --- /dev/null +++ b/tests/core/parameters/test_parameter_metadata_clone.py @@ -0,0 +1,248 @@ +"""A clone of a parameter tree owns its metadata dicts and shares their values. + +``clone`` on a node, parameter, dated value, scale or bracket gives every +object in the copy its own metadata dict, holding the same values as the +source's. So a key set, updated, popped or replaced on one side never shows +on the other, while a nested value (a list of references, a dict) is one +object for both until a key is replaced. ``interpolate_parameters`` replaces +the ``interpolation`` dict it marks complete for that reason. + +``test_parameter_metadata_clone_property.py`` checks the same over random +trees and edits, against a reference clone that deep-copies metadata. +""" + +import copy + +import pytest + +from policyengine_core.country_template import CountryTaxBenefitSystem +from policyengine_core.country_template import entities as template_entities +from policyengine_core.parameters import ( + Parameter, + ParameterAtInstant, + ParameterNode, + interpolate_parameters, + propagate_parameter_metadata, +) +from policyengine_core.reforms import Reform +from policyengine_core.taxbenefitsystems import TaxBenefitSystem +from tests.fixtures.parameter_metadata_clones import build, snapshot + +# Each kind of object that carries metadata, and how to reach it in the +# tree from ``build``. +KINDS = { + "node": lambda root: root.benefit, + "parameter": lambda root: root.benefit.amount, + "value": lambda root: root.benefit.amount.values_list[-1], + "scale": lambda root: root.tax, + "bracket": lambda root: root.tax.brackets[0], + "bracket component": lambda root: root.tax.brackets[0].rate, +} + +# Each way to change a metadata dict through its own methods, or by +# replacing it. +EDITS = { + "set a key": lambda metadata: metadata.__setitem__("label", "Edited"), + "replace a nested value": lambda metadata: metadata.__setitem__( + "reference", [{"title": "Edited", "href": "https://example.gov/edited"}] + ), + "update": lambda metadata: metadata.update(label="Edited", added=[1]), + "setdefault": lambda metadata: metadata.setdefault("added", {"x": 1}), + "pop": lambda metadata: metadata.pop("reference", None), + "delete": lambda metadata: metadata.__delitem__(next(iter(metadata))), + "clear": lambda metadata: metadata.clear(), +} + + +def _metadata_of(item): + if not item.metadata: + # Give the object something to edit and to share. + item.metadata.update(label="Label", reference=[{"title": "Act"}]) + return item.metadata + + +@pytest.mark.parametrize("edit", EDITS) +@pytest.mark.parametrize("kind", KINDS) +@pytest.mark.parametrize("edited_side", ["clone", "source"]) +def test_a_metadata_edit_stays_on_its_side(kind, edit, edited_side): + source = build() + _metadata_of(KINDS[kind](source)) + clone = source.clone() + edited, other = (clone, source) if edited_side == "clone" else (source, clone) + other_before = snapshot(other) + edited_metadata = KINDS[kind](edited).metadata + expected = copy.deepcopy(edited_metadata) + EDITS[edit](expected) + + EDITS[edit](edited_metadata) + + assert KINDS[kind](edited).metadata == expected + assert snapshot(other) == other_before + + +@pytest.mark.parametrize("kind", KINDS) +def test_a_clone_shares_nested_metadata_values_with_its_source(kind): + """The documented rule: own dict, shared values (no deep copy).""" + source = build() + item = KINDS[kind](source) + _metadata_of(item) + cloned = KINDS[kind](source.clone()) + + assert cloned.metadata is not item.metadata + assert cloned.metadata == item.metadata + for key, value in item.metadata.items(): + assert cloned.metadata[key] is value + + +@pytest.mark.parametrize( + "make", + [ + lambda: Parameter("p", {"2015-01-01": 1}), + lambda: ParameterAtInstant("p", "2015-01-01", data=1), + lambda: ParameterNode("n", data={"p": {"2015-01-01": 1}}), + ], + ids=["parameter", "value", "node"], +) +def test_a_clone_copies_metadata_as_copy_copy_does(make): + """``None`` stays ``None`` and a dict subclass keeps its type, as before.""" + + class Metadata(dict): + pass + + item = make() + item.metadata = None + assert item.clone().metadata is None + + item.metadata = Metadata(label="Label") + cloned = item.clone() + assert type(cloned.metadata) is Metadata + assert cloned.metadata == {"label": "Label"} + assert cloned.metadata is not item.metadata + + +def test_interpolating_a_tree_interpolates_the_clone_it_holds(): + """The shape ``TaxBenefitSystem.__init__`` builds. + + It adds a clone of the tree as ``parameters.baseline``, then interpolates + the whole tree. The clone shares the source's ``interpolation`` dict, so + marking that dict complete in place would skip whichever copy came second. + """ + root = build() + root.add_child("baseline", root.clone()) + + interpolate_parameters(root) + + for amount in (root.benefit.amount, root.baseline.benefit.amount): + assert amount("2016-06-01") == 110 + assert amount("2018-06-01") == 130 + assert amount.metadata["interpolation"]["completed"] is True + + +def test_a_tax_benefit_system_interpolates_its_baseline_copy(tmp_path): + (tmp_path / "amount.yaml").write_text( + "description: An interpolated amount.\n" + "metadata:\n" + " interpolation:\n" + " interval: year\n" + "values:\n" + " 2015-01-01: 100\n" + " 2019-01-01: 140\n" + ) + + class System(TaxBenefitSystem): + entities = template_entities.entities + parameters_dir = str(tmp_path) + + system = System() + + assert system.parameters.amount("2016-06-01") == 110 + assert system.parameters.baseline.amount("2016-06-01") == 110 + + +def test_interpolating_a_clone_leaves_its_source_uninterpolated(): + source = build() + clone = source.clone() + source_before = snapshot(source) + + interpolate_parameters(clone) + + assert clone.benefit.amount("2016-06-01") == 110 + assert snapshot(source) == source_before + assert "completed" not in source.benefit.amount.metadata["interpolation"] + # And the source still interpolates when asked. + interpolate_parameters(source) + assert source.benefit.amount("2016-06-01") == 110 + + +def test_a_clone_of_an_interpolated_tree_is_not_interpolated_again(): + source = interpolate_parameters(build()) + clone = source.clone() + values_before = snapshot(clone) + + interpolate_parameters(clone) + + assert snapshot(clone) == values_before + + +@pytest.mark.parametrize("edited_side", ["clone", "source"]) +def test_propagating_metadata_on_one_side_leaves_the_other_alone(edited_side): + source = build() + clone = source.clone() + edited, other = (clone, source) if edited_side == "clone" else (source, clone) + other_before = snapshot(other) + + propagate_parameter_metadata(edited) + + assert edited.benefit.amount.metadata["notes"] == {"sources": ["benefit.pdf"]} + assert snapshot(other) == other_before + + +@pytest.mark.parametrize("edited_side", ["clone", "source"]) +def test_tax_benefit_system_clones_keep_their_own_metadata(edited_side): + source = CountryTaxBenefitSystem() + clone = source.clone() + edited, other = (clone, source) if edited_side == "clone" else (source, clone) + other_before = snapshot(other.parameters) + tree = edited.parameters + + tree.taxes.income_tax_rate.metadata["label"] = "Edited" + tree.taxes.income_tax_rate.values_list[0].metadata["note"] = "Edited" + tree.taxes.social_security_contribution.metadata.pop("reference", None) + tree.benefits.metadata.update(label="Edited", reference=[]) + + assert snapshot(other.parameters) == other_before + assert tree.taxes.income_tax_rate.metadata["label"] == "Edited" + + +def test_a_reform_that_edits_metadata_leaves_its_baseline_alone(): + baseline = CountryTaxBenefitSystem() + baseline_before = snapshot(baseline.parameters) + + class EditMetadata(Reform): + def apply(self): + rate = self.parameters.taxes.income_tax_rate + rate.metadata["label"] = "Reformed" + rate.values_list[0].metadata["note"] = "Reformed" + rate.update(period="year:2017:1", value=0.3) + + reformed = EditMetadata(baseline) + + assert reformed.parameters.taxes.income_tax_rate.metadata["label"] == "Reformed" + assert snapshot(baseline.parameters) == baseline_before + + +def test_cloning_a_parameter_tree_deep_copies_no_metadata(monkeypatch): + """The copy that dominated the cost of cloning a large tree is gone.""" + tree = CountryTaxBenefitSystem().parameters + calls = [] + deepcopy = copy.deepcopy + + def counting_deepcopy(*args, **kwargs): + calls.append(args[0]) + return deepcopy(*args, **kwargs) + + monkeypatch.setattr(copy, "deepcopy", counting_deepcopy) + clone = tree.clone() + + assert calls == [] + assert snapshot(clone) == snapshot(tree) diff --git a/tests/core/parameters/test_parameter_metadata_clone_property.py b/tests/core/parameters/test_parameter_metadata_clone_property.py new file mode 100644 index 000000000..557069ce4 --- /dev/null +++ b/tests/core/parameters/test_parameter_metadata_clone_property.py @@ -0,0 +1,148 @@ +"""Properties of parameter tree clones: each copy is a tree of its own. + +For any chain of clones of a parameter tree (a clone of the source, or of +another clone) and any edits made along the way to any tree in the chain, +through ``Parameter.update``, a value assignment, a value removal, the +metadata dict's own methods, replacing nested metadata values, and Core's +``propagate_parameter_metadata`` and ``interpolate_parameters``: + +* every tree reads as a tree built fresh from the same data and given the + same edits (its own, and those of the trees it was cloned from before it + was cloned), so no edit reaches a tree it was not made on; +* ``clone``, which shares nested metadata values, and ``deep_clone``, which + deep-copies them as Core used to, leave every tree in the same state + (differential). + +A nested metadata value edited in place, rather than replaced, is visible +through every tree that shares it: that is the documented contract and is +not one of the edits here. ``test_parameter_metadata_clone.py`` has the +examples. +""" + +import pytest + +# The smoke job installs Core without the dev extra but collects every module. +pytest.importorskip("hypothesis") + +from hypothesis import given, settings # noqa: E402 +from hypothesis import strategies as st # noqa: E402 + +from tests.fixtures.parameter_metadata_clones import ( # noqa: E402 + apply_edit, + build, + deep_clone, + snapshot, +) + +# Positions are taken modulo the number of objects in the tree. +POSITIONS = st.integers(min_value=0, max_value=60) +KEYS = st.sampled_from( + ["label", "unit", "reference", "notes", "added", "propagate_metadata_to_children"] +) +LEAVES = st.one_of(st.text(max_size=3), st.integers(-3, 3), st.booleans()) +METADATA_VALUES = st.recursive( + LEAVES, + lambda inner: st.one_of( + st.lists(inner, max_size=2), + st.dictionaries(st.sampled_from(["title", "href", "x"]), inner, max_size=2), + ), + max_leaves=4, +) +NUMBERS = st.one_of(st.integers(0, 500), st.floats(0, 500, allow_nan=False)) + +EDITS = st.one_of( + st.tuples(st.just("update"), POSITIONS, st.integers(2014, 2024), NUMBERS), + st.tuples(st.just("set value"), POSITIONS, POSITIONS, NUMBERS), + st.tuples(st.just("remove value"), POSITIONS, POSITIONS), + st.tuples(st.just("set key"), POSITIONS, KEYS, METADATA_VALUES), + st.tuples( + st.just("update keys"), + POSITIONS, + st.dictionaries(KEYS, METADATA_VALUES, max_size=2), + ), + st.tuples(st.just("setdefault key"), POSITIONS, KEYS, METADATA_VALUES), + st.tuples(st.just("pop key"), POSITIONS, KEYS), + st.tuples(st.just("replace values"), POSITIONS), + st.tuples(st.just("propagate")), + st.tuples(st.just("interpolate")), +) + + +@st.composite +def chains(draw): + """Steps on a growing list of trees, the first being the source. + + A step either clones a tree in the list (appending the clone) or edits + one. Each step names its tree by position in the list. + """ + steps = [] + count = 1 + for _ in range(draw(st.integers(min_value=1, max_value=8))): + target = draw(st.integers(min_value=0, max_value=count - 1)) + if draw(st.booleans()): + steps.append(("clone", target)) + count += 1 + else: + steps.append(("edit", target, draw(EDITS))) + return steps + + +def _run(steps, clone): + """Run ``steps`` with ``clone``; return each tree and its edit history.""" + trees = [build()] + histories = [[]] + for step in steps: + if step[0] == "clone": + trees.append(clone(trees[step[1]])) + histories.append(list(histories[step[1]])) + else: + apply_edit(trees[step[1]], step[2]) + histories[step[1]].append(step[2]) + return trees, histories + + +def _fresh(history): + tree = build() + for edit in history: + apply_edit(tree, edit) + return tree + + +_SETTINGS = dict(max_examples=300, deadline=None, derandomize=True) + + +@settings(**_SETTINGS) +@given(steps=chains()) +def test_every_tree_in_a_clone_chain_reads_as_a_fresh_tree(steps): + trees, histories = _run(steps, lambda tree: tree.clone()) + + for tree, history in zip(trees, histories): + assert snapshot(tree) == snapshot(_fresh(history)) + + +@settings(**_SETTINGS) +@given(steps=chains()) +def test_sharing_nested_metadata_matches_deep_copying_it(steps): + shared, _ = _run(steps, lambda tree: tree.clone()) + deep, _ = _run(steps, deep_clone) + + assert [snapshot(tree) for tree in shared] == [snapshot(tree) for tree in deep] + + +@settings(**_SETTINGS) +@given( + edits=st.lists(EDITS, min_size=1, max_size=6), + edit_clone=st.booleans(), +) +def test_edits_to_one_side_never_change_the_other(edits, edit_clone): + """The two-tree case, checked against the untouched side's own snapshot.""" + source = build() + clone = source.clone() + edited, other = (clone, source) if edit_clone else (source, clone) + other_before = snapshot(other) + + for edit in edits: + apply_edit(edited, edit) + + assert snapshot(other) == other_before + assert snapshot(edited) == snapshot(_fresh(edits)) diff --git a/tests/fixtures/parameter_metadata_clones.py b/tests/fixtures/parameter_metadata_clones.py new file mode 100644 index 000000000..bfcb0b2ad --- /dev/null +++ b/tests/fixtures/parameter_metadata_clones.py @@ -0,0 +1,200 @@ +"""Parameter trees, copies and edits for the parameter clone tests. + +``tree_data`` builds a tree with metadata on every kind of object: nodes, +parameters, dated values, scales and brackets. Some metadata values are +nested (a list of reference dicts, a dict holding a list), which is what a +clone shares with its source. + +``deep_clone`` copies a tree the way Core did before clones shared nested +metadata: every metadata dict is deep-copied. It is the reference the +differential tests compare ``clone`` against. + +``snapshot`` turns a tree into plain data, so two trees, or one tree before +and after an edit, can be compared with ``==``. + +``apply_edit`` makes one edit through an API that callers use on a tree: +``Parameter.update``, assigning a value, removing one, the metadata dict's +own methods, replacing a nested metadata value with a new one, and Core's +``propagate_parameter_metadata`` and ``interpolate_parameters``. +""" + +import copy + +from policyengine_core import commons +from policyengine_core.parameters import ( + Parameter, + ParameterAtInstant, + ParameterNode, + ParameterScale, + interpolate_parameters, + propagate_parameter_metadata, +) + +# ``snapshot`` reads each parameter at these instants. +INSTANTS = ["2014-06-01", "2016-01-01", "2018-07-01", "2020-01-01", "2022-01-01"] + + +def reference(label): + return [{"title": f"{label} act", "href": f"https://example.gov/{label}"}] + + +def tree_data(): + """A tree with metadata, nested where it can be, on every kind of object.""" + return { + "metadata": {"label": "Root", "reference": reference("root")}, + "benefit": { + "metadata": { + "label": "Benefit", + "propagate_metadata_to_children": True, + "notes": {"sources": ["benefit.pdf"]}, + }, + "amount": { + "description": "An interpolated amount.", + "metadata": { + "unit": "currency-USD", + "reference": reference("amount"), + "interpolation": {"interval": "year"}, + }, + "values": { + "2015-01-01": { + "value": 100, + "metadata": {"reference": reference("2015")}, + }, + "2019-01-01": 140, + }, + }, + "eligible": {"2015-01-01": True, "2020-01-01": False}, + }, + "tax": { + "metadata": {"label": "Tax", "reference": reference("scale")}, + "brackets": [ + { + "metadata": {"notes": {"sources": ["bracket.pdf"]}}, + "threshold": {"values": {"2015-01-01": 0}}, + "rate": { + "metadata": {"reference": reference("rate")}, + "values": {"2015-01-01": 0.1, "2018-01-01": 0.12}, + }, + }, + { + "threshold": {"values": {"2015-01-01": 1_000}}, + "rate": {"values": {"2015-01-01": 0.2}}, + }, + ], + }, + } + + +def build(data=None): + return ParameterNode(data=copy.deepcopy(tree_data() if data is None else data)) + + +def deep_clone(item): + """Clone ``item`` as Core did before clones shared nested metadata.""" + clone = commons.empty_clone(item) + clone.__dict__ = item.__dict__.copy() + clone.metadata = copy.deepcopy(item.metadata) + if isinstance(item, ParameterAtInstant): + return clone + if isinstance(item, Parameter): + clone.values_list = [deep_clone(value) for value in item.values_list] + return clone + if isinstance(item, ParameterScale): + clone.brackets = [deep_clone(bracket) for bracket in item.brackets] + for bracket in clone.brackets: + bracket.parent = clone + return clone + clone.children = {key: deep_clone(child) for key, child in item.children.items()} + for key, child in clone.children.items(): + setattr(clone, key, child) + child.parent = clone + clone._at_instant_cache = {} + return clone + + +def objects(root): + """Every object in the tree, the root first, in a fixed order.""" + found = [root] + for item in root.get_descendants(): + found.append(item) + if isinstance(item, Parameter): + found.extend(item.values_list) + return found + + +def parameters(root): + return [item for item in root.get_descendants() if isinstance(item, Parameter)] + + +def snapshot(item): + """``item`` and everything below it as plain data.""" + metadata = copy.deepcopy(item.metadata) + if isinstance(item, ParameterAtInstant): + return ("value", item.instant_str, copy.deepcopy(item.value), metadata) + if isinstance(item, Parameter): + return ( + "parameter", + metadata, + [snapshot(value) for value in item.values_list], + # Read through the parameter as well as its list. + [item._get_at_instant(instant) for instant in INSTANTS], + ) + if isinstance(item, ParameterScale): + return ("scale", metadata, [snapshot(bracket) for bracket in item.brackets]) + return ( + "node", + metadata, + {key: snapshot(child) for key, child in item.children.items()}, + ) + + +def replaced(value): + """A new value in place of a metadata value, leaving the old one alone.""" + if isinstance(value, dict): + return {**value, "replaced": True} + if isinstance(value, list): + return [*value, "replaced"] + return ("replaced", value) + + +def apply_edit(root, edit): + """Make ``edit`` on the tree under ``root``. + + Positions index ``objects(root)`` or ``parameters(root)`` modulo their + length, so any position names something in any tree. + """ + kind = edit[0] + if kind == "propagate": + propagate_parameter_metadata(root) + return + if kind == "interpolate": + interpolate_parameters(root) + return + if kind in ("update", "set value", "remove value"): + parameter = parameters(root)[edit[1] % len(parameters(root))] + if kind == "update": + _, _, year, value = edit + parameter.update(period=f"year:{year}:1", value=value) + elif kind == "set value": + _, _, position, value = edit + values = parameter.values_list + values[position % len(values)].value = value + elif len(parameter.values_list) > 1: + parameter.values_list.pop(edit[2] % len(parameter.values_list)) + return + found = objects(root) + metadata = found[edit[1] % len(found)].metadata + if kind == "set key": + metadata[edit[2]] = copy.deepcopy(edit[3]) + elif kind == "update keys": + metadata.update(copy.deepcopy(edit[2])) + elif kind == "setdefault key": + metadata.setdefault(edit[2], copy.deepcopy(edit[3])) + elif kind == "pop key": + metadata.pop(edit[2], None) + elif kind == "replace values": + # Change every nested value the documented way: a new value per key. + for key, value in list(metadata.items()): + metadata[key] = replaced(value) + else: + raise ValueError(f"Unknown edit {edit!r}") From 449d2d4b932640cf533945609d8eef9d2a69d45d Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 24 Oct 2026 07:37:51 -0400 Subject: [PATCH 2/6] Test that every child a node propagates interpolation to is interpolated propagate_parameter_metadata hands each descendant the node's own interpolation dict, so marking it complete in place skipped every child after the first. Replacing the dict fixes that too; no PolicyEngine country model uses propagated interpolation. Co-Authored-By: Claude Opus 5.5 --- ...are-parameter-metadata-on-clone.changed.md | 2 +- .../test_parameter_metadata_clone.py | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/changelog.d/share-parameter-metadata-on-clone.changed.md b/changelog.d/share-parameter-metadata-on-clone.changed.md index c468da864..c923cb7b3 100644 --- a/changelog.d/share-parameter-metadata-on-clone.changed.md +++ b/changelog.d/share-parameter-metadata-on-clone.changed.md @@ -1 +1 @@ -Parameter clones copy each metadata dict without deep-copying the values in it, which roughly halves the time to clone a large tax-benefit system; `interpolate_parameters` now replaces the `interpolation` dict it marks complete instead of editing it. +Parameter clones copy each metadata dict without deep-copying the values in it, which makes cloning a large tax-benefit system faster and smaller. `interpolate_parameters` now replaces the `interpolation` dict it marks complete instead of editing it, so it also interpolates every parameter a node propagates `interpolation` to, not only the first. diff --git a/tests/core/parameters/test_parameter_metadata_clone.py b/tests/core/parameters/test_parameter_metadata_clone.py index eed9595c3..75a3d2783 100644 --- a/tests/core/parameters/test_parameter_metadata_clone.py +++ b/tests/core/parameters/test_parameter_metadata_clone.py @@ -159,6 +159,31 @@ class System(TaxBenefitSystem): assert system.parameters.baseline.amount("2016-06-01") == 110 +def test_every_parameter_a_node_propagates_interpolation_to_is_interpolated(): + """Propagation gives each child the node's ``interpolation`` dict. + + Marking that one dict complete in place skipped every child after the + first. + """ + root = ParameterNode( + data={ + "node": { + "metadata": { + "propagate_metadata_to_children": True, + "interpolation": {"interval": "year"}, + }, + "a": {"values": {"2015-01-01": 100, "2019-01-01": 140}}, + "b": {"values": {"2015-01-01": 200, "2019-01-01": 280}}, + } + } + ) + + interpolate_parameters(propagate_parameter_metadata(root)) + + assert root.node.a("2016-06-01") == 110 + assert root.node.b("2016-06-01") == 220 + + def test_interpolating_a_clone_leaves_its_source_uninterpolated(): source = build() clone = source.clone() From 6f5e46b87847be8439aeba10a2a6f4d870f3debb Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 30 Oct 2026 21:04:56 -0400 Subject: [PATCH 3/6] Record interpolation on the parameter, not in its metadata Replacing the parameter's `interpolation` dict left the propagating node's dict unmarked, so `TaxBenefitSystem.__init__`'s final `propagate_parameter_metadata` overwrote the parameter's marked copy and a second `interpolate_parameters` added every entry again. Completion belongs to a parameter's values, while the `interpolation` dict is shared by its clones and by every parameter under a propagating node. So `Parameter.interpolated` now holds it, and `interpolate_parameters` writes no metadata. Add the constructor sequence as a test and as a property, an `interpolation` node to the shared fixture, and a differential against the in-place version's values. Correct the docs on which dated values hold their parameter's metadata. Co-Authored-By: Claude Opus 5.5 --- ...are-parameter-metadata-on-clone.changed.md | 2 +- docs/usage/parameters.md | 2 +- .../operations/interpolate_parameters.py | 16 ++-- policyengine_core/parameters/parameter.py | 1 + .../test_parameter_metadata_clone.py | 73 +++++++++++++++++-- .../test_parameter_metadata_clone_property.py | 68 ++++++++++++++++- tests/fixtures/parameter_metadata_clones.py | 59 ++++++++++++++- 7 files changed, 200 insertions(+), 21 deletions(-) diff --git a/changelog.d/share-parameter-metadata-on-clone.changed.md b/changelog.d/share-parameter-metadata-on-clone.changed.md index c923cb7b3..063df1d72 100644 --- a/changelog.d/share-parameter-metadata-on-clone.changed.md +++ b/changelog.d/share-parameter-metadata-on-clone.changed.md @@ -1 +1 @@ -Parameter clones copy each metadata dict without deep-copying the values in it, which makes cloning a large tax-benefit system faster and smaller. `interpolate_parameters` now replaces the `interpolation` dict it marks complete instead of editing it, so it also interpolates every parameter a node propagates `interpolation` to, not only the first. +Parameter clones copy each metadata dict without deep-copying the values in it, which makes cloning a large tax-benefit system faster and smaller. `interpolate_parameters` now records its work in `Parameter.interpolated` instead of writing `completed` into the `interpolation` metadata, so it interpolates every parameter a node propagates `interpolation` to, not only the first. diff --git a/docs/usage/parameters.md b/docs/usage/parameters.md index e876129ac..f4758a22e 100644 --- a/docs/usage/parameters.md +++ b/docs/usage/parameters.md @@ -65,7 +65,7 @@ metadata: parameter.metadata["reference"] = [*parameter.metadata["reference"], new_reference] ``` -The same holds within one tree: a parameter's dated values hold the parameter's metadata values, and `propagate_parameter_metadata` gives a node's descendants the node's values. +The same holds within one tree: a dated value written in dictionary form (`value:`, with or without `metadata:`) holds its parameter's metadata values, and `propagate_parameter_metadata` gives a node's descendants the node's values. ### Metadata for parameters diff --git a/policyengine_core/parameters/operations/interpolate_parameters.py b/policyengine_core/parameters/operations/interpolate_parameters.py index 6b4a0ef29..6b8b51236 100644 --- a/policyengine_core/parameters/operations/interpolate_parameters.py +++ b/policyengine_core/parameters/operations/interpolate_parameters.py @@ -9,6 +9,8 @@ def interpolate_parameters(root: ParameterNode) -> ParameterNode: """Interpolates parameters according to their metadata. + Each parameter interpolates once, however many times this runs. + Args: root (ParameterNode): The root of the parameter tree. @@ -17,9 +19,7 @@ def interpolate_parameters(root: ParameterNode) -> ParameterNode: """ for parameter in root.get_descendants(): if isinstance(parameter, Parameter): - if "interpolation" in parameter.metadata and not parameter.metadata[ - "interpolation" - ].get("completed", False): + if "interpolation" in parameter.metadata and not parameter.interpolated: interpolated_entries = [] for i in range(len(parameter.values_list) - 1): # For each gap in parameter values @@ -56,10 +56,8 @@ def interpolate_parameters(root: ParameterNode) -> ParameterNode: for entry in interpolated_entries: parameter.values_list.append(entry) parameter.values_list.sort(key=lambda x: x.instant_str, reverse=True) - # Replace the dict rather than edit it: a clone of this - # parameter shares it (see ParameterAtInstant.clone). - parameter.metadata["interpolation"] = { - **parameter.metadata["interpolation"], - "completed": True, - } + # Mark the parameter, not its ``interpolation`` dict. Its clones + # and every parameter under a propagating node share that dict, + # and propagating metadata again replaces this parameter's. + parameter.interpolated = True return root diff --git a/policyengine_core/parameters/parameter.py b/policyengine_core/parameters/parameter.py index 9bc5eb27d..5aa60c52e 100644 --- a/policyengine_core/parameters/parameter.py +++ b/policyengine_core/parameters/parameter.py @@ -105,6 +105,7 @@ def __init__(self, name: str, data: dict, file_path: Optional[str] = None) -> No self.values_list: List[ParameterAtInstant] = values_list self.modified: bool = False + self.interpolated: bool = False def __repr__(self): return os.linesep.join( diff --git a/tests/core/parameters/test_parameter_metadata_clone.py b/tests/core/parameters/test_parameter_metadata_clone.py index 75a3d2783..4da8edf13 100644 --- a/tests/core/parameters/test_parameter_metadata_clone.py +++ b/tests/core/parameters/test_parameter_metadata_clone.py @@ -4,8 +4,8 @@ object in the copy its own metadata dict, holding the same values as the source's. So a key set, updated, popped or replaced on one side never shows on the other, while a nested value (a list of references, a dict) is one -object for both until a key is replaced. ``interpolate_parameters`` replaces -the ``interpolation`` dict it marks complete for that reason. +object for both until a key is replaced. ``interpolate_parameters`` marks the +parameter it has interpolated, not the ``interpolation`` dict, for that reason. ``test_parameter_metadata_clone_property.py`` checks the same over random trees and edits, against a reference clone that deep-copies metadata. @@ -135,7 +135,7 @@ def test_interpolating_a_tree_interpolates_the_clone_it_holds(): for amount in (root.benefit.amount, root.baseline.benefit.amount): assert amount("2016-06-01") == 110 assert amount("2018-06-01") == 130 - assert amount.metadata["interpolation"]["completed"] is True + assert amount.interpolated def test_a_tax_benefit_system_interpolates_its_baseline_copy(tmp_path): @@ -159,11 +159,63 @@ class System(TaxBenefitSystem): assert system.parameters.baseline.amount("2016-06-01") == 110 +def test_a_tax_benefit_system_interpolates_each_parameter_once(tmp_path): + """``TaxBenefitSystem.__init__`` propagates metadata again after interpolating. + + That gives each parameter the node's ``interpolation`` dict again, so a + mark on the parameter's own copy of the dict would be lost, and the next + interpolation would add every entry a second time. + """ + node = tmp_path / "node" + node.mkdir() + (node / "index.yaml").write_text( + "metadata:\n" + " propagate_metadata_to_children: true\n" + " interpolation:\n" + " interval: year\n" + ) + for name, start, end in (("a", 100, 140), ("b", 200, 280)): + (node / f"{name}.yaml").write_text( + "description: An amount.\n" + "values:\n" + f" 2015-01-01: {start}\n" + f" 2019-01-01: {end}\n" + ) + + class System(TaxBenefitSystem): + entities = template_entities.entities + parameters_dir = str(tmp_path) + + system = System() + before = snapshot(system.parameters) + + interpolate_parameters(system.parameters) + + assert snapshot(system.parameters) == before + for tree in (system.parameters, system.parameters.baseline): + assert len(tree.node.a.values_list) == 5 + assert tree.node.a("2016-06-01") == 110 + assert tree.node.b("2016-06-01") == 220 + + +def test_interpolating_writes_no_metadata(): + root = propagate_parameter_metadata(build()) + root.add_child("baseline", root.clone()) + metadata_before = [ + (item, copy.deepcopy(item.metadata)) for item in root.get_descendants() + ] + + interpolate_parameters(root) + + assert root.allowance.couple("2016-06-01") == 220 + for item, metadata in metadata_before: + assert item.metadata == metadata + + def test_every_parameter_a_node_propagates_interpolation_to_is_interpolated(): """Propagation gives each child the node's ``interpolation`` dict. - Marking that one dict complete in place skipped every child after the - first. + Marking that one dict complete skipped every child after the first. """ root = ParameterNode( data={ @@ -193,15 +245,20 @@ def test_interpolating_a_clone_leaves_its_source_uninterpolated(): assert clone.benefit.amount("2016-06-01") == 110 assert snapshot(source) == source_before - assert "completed" not in source.benefit.amount.metadata["interpolation"] + assert not source.benefit.amount.interpolated # And the source still interpolates when asked. interpolate_parameters(source) assert source.benefit.amount("2016-06-01") == 110 -def test_a_clone_of_an_interpolated_tree_is_not_interpolated_again(): +@pytest.mark.parametrize( + "copy_tree", + [lambda tree: tree.clone(), copy.deepcopy], + ids=["clone", "deepcopy"], +) +def test_a_copy_of_an_interpolated_tree_is_not_interpolated_again(copy_tree): source = interpolate_parameters(build()) - clone = source.clone() + clone = copy_tree(source) values_before = snapshot(clone) interpolate_parameters(clone) diff --git a/tests/core/parameters/test_parameter_metadata_clone_property.py b/tests/core/parameters/test_parameter_metadata_clone_property.py index 557069ce4..40704e1b1 100644 --- a/tests/core/parameters/test_parameter_metadata_clone_property.py +++ b/tests/core/parameters/test_parameter_metadata_clone_property.py @@ -11,7 +11,12 @@ was cloned), so no edit reaches a tree it was not made on; * ``clone``, which shares nested metadata values, and ``deep_clone``, which deep-copies them as Core used to, leave every tree in the same state - (differential). + (differential); +* after ``TaxBenefitSystem.__init__``'s propagate, interpolate, propagate, + interpolating again changes nothing. + +``interpolate_parameters`` gives the values Core's in-place version gave, on +parameters that each have their own ``interpolation`` dict (differential). A nested metadata value edited in place, rather than replaced, is visible through every tree that shares it: that is the documented contract and is @@ -19,6 +24,8 @@ examples. """ +from datetime import date + import pytest # The smoke job installs Core without the dev extra but collects every module. @@ -27,10 +34,15 @@ from hypothesis import given, settings # noqa: E402 from hypothesis import strategies as st # noqa: E402 +from policyengine_core.parameters import ( # noqa: E402 + ParameterNode, + interpolate_parameters, +) from tests.fixtures.parameter_metadata_clones import ( # noqa: E402 apply_edit, build, deep_clone, + interpolate_in_place, snapshot, ) @@ -63,6 +75,7 @@ st.tuples(st.just("setdefault key"), POSITIONS, KEYS, METADATA_VALUES), st.tuples(st.just("pop key"), POSITIONS, KEYS), st.tuples(st.just("replace values"), POSITIONS), + st.tuples(st.just("set interpolation"), POSITIONS), st.tuples(st.just("propagate")), st.tuples(st.just("interpolate")), ) @@ -146,3 +159,56 @@ def test_edits_to_one_side_never_change_the_other(edits, edit_clone): assert snapshot(other) == other_before assert snapshot(edited) == snapshot(_fresh(edits)) + + +@settings(**_SETTINGS) +@given(steps=chains()) +def test_interpolating_a_processed_tree_again_changes_nothing(steps): + trees, _ = _run(steps, lambda tree: tree.clone()) + + for tree in trees: + for step in ("propagate", "interpolate", "propagate"): + apply_edit(tree, (step,)) + processed = snapshot(tree) + apply_edit(tree, ("interpolate",)) + assert snapshot(tree) == processed + + +_DATES = st.dates(min_value=date(2015, 1, 1), max_value=date(2022, 12, 1)).map( + lambda day: day.replace(day=1).isoformat() +) + + +def _tree(interval, values_by_parameter): + return ParameterNode( + data={ + f"parameter_{position}": { + "metadata": {"interpolation": {"interval": interval}}, + "values": dict(values), + } + for position, values in enumerate(values_by_parameter) + } + ) + + +@settings(**_SETTINGS) +@given( + interval=st.sampled_from(["year", "month"]), + values_by_parameter=st.lists( + st.dictionaries(_DATES, st.integers(0, 500), min_size=1, max_size=4), + min_size=1, + max_size=3, + ), +) +def test_interpolated_values_match_the_in_place_reference( + interval, values_by_parameter +): + interpolated = interpolate_parameters(_tree(interval, values_by_parameter)) + reference = interpolate_in_place(_tree(interval, values_by_parameter)) + + for parameter, expected in zip( + interpolated.children.values(), reference.children.values() + ): + assert [(item.instant_str, item.value) for item in parameter.values_list] == [ + (item.instant_str, item.value) for item in expected.values_list + ] diff --git a/tests/fixtures/parameter_metadata_clones.py b/tests/fixtures/parameter_metadata_clones.py index bfcb0b2ad..64a6817c5 100644 --- a/tests/fixtures/parameter_metadata_clones.py +++ b/tests/fixtures/parameter_metadata_clones.py @@ -7,7 +7,8 @@ ``deep_clone`` copies a tree the way Core did before clones shared nested metadata: every metadata dict is deep-copied. It is the reference the -differential tests compare ``clone`` against. +differential tests compare ``clone`` against. ``interpolate_in_place`` is the +same for ``interpolate_parameters``. ``snapshot`` turns a tree into plain data, so two trees, or one tree before and after an edit, can be compared with ``==``. @@ -29,6 +30,7 @@ interpolate_parameters, propagate_parameter_metadata, ) +from policyengine_core.periods import instant # ``snapshot`` reads each parameter at these instants. INSTANTS = ["2014-06-01", "2016-01-01", "2018-07-01", "2020-01-01", "2022-01-01"] @@ -65,6 +67,14 @@ def tree_data(): }, "eligible": {"2015-01-01": True, "2020-01-01": False}, }, + "allowance": { + "metadata": { + "propagate_metadata_to_children": True, + "interpolation": {"interval": "year"}, + }, + "single": {"values": {"2015-01-01": 100, "2019-01-01": 140}}, + "couple": {"values": {"2015-01-01": 200, "2019-01-01": 280}}, + }, "tax": { "metadata": {"label": "Tax", "reference": reference("scale")}, "brackets": [ @@ -112,6 +122,50 @@ def deep_clone(item): return clone +def interpolate_in_place(root): + """``interpolate_parameters`` as Core had it, with its completion marker. + + The reference for the values interpolation gives. It marks a dict that + several objects can share, so compare it only on parameters that each + have their own. + """ + for parameter in root.get_descendants(): + if isinstance(parameter, Parameter): + if "interpolation" in parameter.metadata and not parameter.metadata[ + "interpolation" + ].get("completed", False): + interpolated_entries = [] + for i in range(len(parameter.values_list) - 1): + start = instant(parameter.values_list[::-1][i].instant_str) + num_intervals = 1 + interval_size = parameter.metadata["interpolation"]["interval"] + parameter_dates = [ + at_instant.instant_str for at_instant in parameter.values_list + ] + while ( + str(start.offset(num_intervals, interval_size)) + < parameter_dates[0] + ): + num_intervals += 1 + for j in range(1, num_intervals): + start_str = str(start.offset(j, interval_size)) + start_value = parameter.values_list[::-1][i].value + end_value = parameter.values_list[::-1][i + 1].value + new_value = ( + start_value + (end_value - start_value) * j / num_intervals + ) + interpolated_entries += [ + ParameterAtInstant( + parameter.name, start_str, data=new_value + ) + ] + for entry in interpolated_entries: + parameter.values_list.append(entry) + parameter.values_list.sort(key=lambda x: x.instant_str, reverse=True) + parameter.metadata["interpolation"]["completed"] = True + return root + + def objects(root): """Every object in the tree, the root first, in a fixed order.""" found = [root] @@ -138,6 +192,7 @@ def snapshot(item): [snapshot(value) for value in item.values_list], # Read through the parameter as well as its list. [item._get_at_instant(instant) for instant in INSTANTS], + item.interpolated, ) if isinstance(item, ParameterScale): return ("scale", metadata, [snapshot(bracket) for bracket in item.brackets]) @@ -192,6 +247,8 @@ def apply_edit(root, edit): metadata.setdefault(edit[2], copy.deepcopy(edit[3])) elif kind == "pop key": metadata.pop(edit[2], None) + elif kind == "set interpolation": + metadata["interpolation"] = {"interval": "year"} elif kind == "replace values": # Change every nested value the documented way: a new value per key. for key, value in list(metadata.items()): From a65e6dd69a656db9633dd889acc0d00e6f9e82b5 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 30 Oct 2026 21:34:34 -0400 Subject: [PATCH 4/6] Default Parameter.interpolated on the class An `__init__` assignment left no attribute on policyengine-uk's `LegacyNIParameter` and other subclasses that skip `Parameter.__init__`, or on a tree pickled before the attribute existed, so interpolating one raised AttributeError. A class default reads the same everywhere and adds nothing to a parameter that never interpolates. Document how to interpolate a parameter again after changing its `interpolation`, which on master took only a new dict, and let the generated edits set a monthly interval. Co-Authored-By: Claude Opus 5.5 --- .../operations/interpolate_parameters.py | 4 +- policyengine_core/parameters/parameter.py | 7 +++- .../test_parameter_metadata_clone.py | 39 +++++++++++++++++++ .../test_parameter_metadata_clone_property.py | 4 +- tests/fixtures/parameter_metadata_clones.py | 2 +- 5 files changed, 52 insertions(+), 4 deletions(-) diff --git a/policyengine_core/parameters/operations/interpolate_parameters.py b/policyengine_core/parameters/operations/interpolate_parameters.py index 6b8b51236..eafe2158f 100644 --- a/policyengine_core/parameters/operations/interpolate_parameters.py +++ b/policyengine_core/parameters/operations/interpolate_parameters.py @@ -9,7 +9,9 @@ def interpolate_parameters(root: ParameterNode) -> ParameterNode: """Interpolates parameters according to their metadata. - Each parameter interpolates once, however many times this runs. + Each parameter interpolates once, however many times this runs. To + interpolate one again after changing its ``interpolation`` metadata, set + its ``interpolated`` attribute to ``False``. Args: root (ParameterNode): The root of the parameter tree. diff --git a/policyengine_core/parameters/parameter.py b/policyengine_core/parameters/parameter.py index 5aa60c52e..171665c19 100644 --- a/policyengine_core/parameters/parameter.py +++ b/policyengine_core/parameters/parameter.py @@ -19,6 +19,8 @@ class Parameter(AtInstantLike): Attributes: values_list: List of the values, in reverse chronological order. + interpolated: Whether :func:`interpolate_parameters` has filled in this + parameter's values. Args: @@ -45,6 +47,10 @@ class Parameter(AtInstantLike): """ + # A class default rather than an ``__init__`` assignment, so a subclass that + # skips ``__init__`` and a tree pickled before this existed can read it. + interpolated: bool = False + def __init__(self, name: str, data: dict, file_path: Optional[str] = None) -> None: self.name: str = name self.file_path: Optional[str] = file_path @@ -105,7 +111,6 @@ def __init__(self, name: str, data: dict, file_path: Optional[str] = None) -> No self.values_list: List[ParameterAtInstant] = values_list self.modified: bool = False - self.interpolated: bool = False def __repr__(self): return os.linesep.join( diff --git a/tests/core/parameters/test_parameter_metadata_clone.py b/tests/core/parameters/test_parameter_metadata_clone.py index 4da8edf13..49b28e504 100644 --- a/tests/core/parameters/test_parameter_metadata_clone.py +++ b/tests/core/parameters/test_parameter_metadata_clone.py @@ -251,6 +251,45 @@ def test_interpolating_a_clone_leaves_its_source_uninterpolated(): assert source.benefit.amount("2016-06-01") == 110 +def test_a_parameter_interpolates_again_once_its_flag_is_cleared(): + root = ParameterNode( + data={ + "amount": { + "metadata": {"interpolation": {"interval": "year"}}, + "values": {"2015-01-01": 100, "2015-07-01": 200}, + } + } + ) + interpolate_parameters(root) + assert len(root.amount.values_list) == 2 + + root.amount.metadata["interpolation"] = {"interval": "month"} + root.amount.interpolated = False + interpolate_parameters(root) + + assert len(root.amount.values_list) == 7 + assert root.amount("2015-06-01") == pytest.approx(100 + 100 * 5 / 6) + + +def test_a_parameter_that_skipped_init_interpolates(): + """A subclass can skip ``Parameter.__init__``. + + So can a parameter pickled before ``interpolated`` existed. Neither has + the attribute on the instance. + """ + source = build().benefit.amount + parameter = Parameter.__new__(Parameter) + parameter.__dict__.update( + {key: value for key, value in vars(source).items() if key != "interpolated"} + ) + root = ParameterNode(data={}) + root.add_child("amount", parameter) + + interpolate_parameters(root) + + assert parameter("2016-06-01") == 110 + + @pytest.mark.parametrize( "copy_tree", [lambda tree: tree.clone(), copy.deepcopy], diff --git a/tests/core/parameters/test_parameter_metadata_clone_property.py b/tests/core/parameters/test_parameter_metadata_clone_property.py index 40704e1b1..b1c5de605 100644 --- a/tests/core/parameters/test_parameter_metadata_clone_property.py +++ b/tests/core/parameters/test_parameter_metadata_clone_property.py @@ -75,7 +75,9 @@ st.tuples(st.just("setdefault key"), POSITIONS, KEYS, METADATA_VALUES), st.tuples(st.just("pop key"), POSITIONS, KEYS), st.tuples(st.just("replace values"), POSITIONS), - st.tuples(st.just("set interpolation"), POSITIONS), + st.tuples( + st.just("set interpolation"), POSITIONS, st.sampled_from(["year", "month"]) + ), st.tuples(st.just("propagate")), st.tuples(st.just("interpolate")), ) diff --git a/tests/fixtures/parameter_metadata_clones.py b/tests/fixtures/parameter_metadata_clones.py index 64a6817c5..aa790301f 100644 --- a/tests/fixtures/parameter_metadata_clones.py +++ b/tests/fixtures/parameter_metadata_clones.py @@ -248,7 +248,7 @@ def apply_edit(root, edit): elif kind == "pop key": metadata.pop(edit[2], None) elif kind == "set interpolation": - metadata["interpolation"] = {"interval": "year"} + metadata["interpolation"] = {"interval": edit[2]} elif kind == "replace values": # Change every nested value the documented way: a new value per key. for key, value in list(metadata.items()): From 4db4c7caed76501038373a4987476146cddd0490 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 30 Oct 2026 21:43:19 -0400 Subject: [PATCH 5/6] Honour a completed key already in interpolation metadata A tree pickled by an older Core carries `completed`, and without this read a second `interpolate_parameters` on it added every entry again. Core still writes no metadata. Co-Authored-By: Claude Opus 5.5 --- .../operations/interpolate_parameters.py | 8 +++++++- .../parameters/test_parameter_metadata_clone.py | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/policyengine_core/parameters/operations/interpolate_parameters.py b/policyengine_core/parameters/operations/interpolate_parameters.py index eafe2158f..2150f590b 100644 --- a/policyengine_core/parameters/operations/interpolate_parameters.py +++ b/policyengine_core/parameters/operations/interpolate_parameters.py @@ -21,7 +21,13 @@ def interpolate_parameters(root: ParameterNode) -> ParameterNode: """ for parameter in root.get_descendants(): if isinstance(parameter, Parameter): - if "interpolation" in parameter.metadata and not parameter.interpolated: + if ( + "interpolation" in parameter.metadata + and not parameter.interpolated + # Core no longer writes this key, but a tree pickled by an + # older Core has it, and so can metadata written by hand. + and not parameter.metadata["interpolation"].get("completed", False) + ): interpolated_entries = [] for i in range(len(parameter.values_list) - 1): # For each gap in parameter values diff --git a/tests/core/parameters/test_parameter_metadata_clone.py b/tests/core/parameters/test_parameter_metadata_clone.py index 49b28e504..555979b6f 100644 --- a/tests/core/parameters/test_parameter_metadata_clone.py +++ b/tests/core/parameters/test_parameter_metadata_clone.py @@ -271,6 +271,22 @@ def test_a_parameter_interpolates_again_once_its_flag_is_cleared(): assert root.amount("2015-06-01") == pytest.approx(100 + 100 * 5 / 6) +def test_a_completed_key_in_the_metadata_still_stops_interpolation(): + """A tree pickled by an older Core carries the key its interpolation wrote.""" + root = ParameterNode( + data={ + "amount": { + "metadata": {"interpolation": {"interval": "year", "completed": True}}, + "values": {"2015-01-01": 100, "2019-01-01": 140}, + } + } + ) + + interpolate_parameters(root) + + assert len(root.amount.values_list) == 2 + + def test_a_parameter_that_skipped_init_interpolates(): """A subclass can skip ``Parameter.__init__``. From 21a8c5f9aecd82b730e510216f63df2dc134a1cf Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 30 Oct 2026 21:57:25 -0400 Subject: [PATCH 6/6] State the old completed key in the interpolation reset docs Co-Authored-By: Claude Opus 5.5 --- .../parameters/operations/interpolate_parameters.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/policyengine_core/parameters/operations/interpolate_parameters.py b/policyengine_core/parameters/operations/interpolate_parameters.py index 2150f590b..6c9a9ad7e 100644 --- a/policyengine_core/parameters/operations/interpolate_parameters.py +++ b/policyengine_core/parameters/operations/interpolate_parameters.py @@ -10,8 +10,8 @@ def interpolate_parameters(root: ParameterNode) -> ParameterNode: """Interpolates parameters according to their metadata. Each parameter interpolates once, however many times this runs. To - interpolate one again after changing its ``interpolation`` metadata, set - its ``interpolated`` attribute to ``False``. + interpolate one again, set its ``interpolated`` attribute to ``False`` and + give it ``interpolation`` metadata without a true ``completed`` key. Args: root (ParameterNode): The root of the parameter tree.