From 426df7b49efa1a6f2bd7428b1009224255ed93f2 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 16:36:54 -0400 Subject: [PATCH 01/10] Test that a reloaded export reads the same identifiers in each input year A simulation built from a frame and given values for a year before its data holds no identifiers there, because policyengine-core carries inputs forward but never back. The frame it exports loads back to a simulation that holds the real identifiers in that year, so formulas that read them compute differently there. The property checks that the simulation and its reloaded export compute the same values in each year given inputs for, both for the identifiers and for every variable whose formula reads one, and that both hold the people's own identifiers there. The examples are Hypothesis's shrunk counterexample (one person, an input three years before the data) and two children of one age, for whom all four identifier readers differed. Further tests cover a month input, a lone identifier, a branch, a situation, and datasets loaded as H5 files load. Refs #2353. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_dataframe_dataset.py | 229 ++++++++++++++++++ 1 file changed, 229 insertions(+) diff --git a/policyengine_uk/tests/test_dataframe_dataset.py b/policyengine_uk/tests/test_dataframe_dataset.py index 6ba665b8fa..abd4e8b301 100644 --- a/policyengine_uk/tests/test_dataframe_dataset.py +++ b/policyengine_uk/tests/test_dataframe_dataset.py @@ -66,7 +66,9 @@ anything, since it also exports every calculated value. """ +import inspect import io +import re import warnings from dataclasses import dataclass from typing import Dict @@ -1404,6 +1406,233 @@ def test_a_frame_over_several_years_exports_the_same_frame_again( assert exported[f"{group}_id__{year}"].tolist() == exact +# A formula that reads an identifier, such as ``person("person_id", period)``. +READS_AN_IDENTIFIER = re.compile( + r"""\(\s*["'](?:%s)["']""" % "|".join(sorted(IDENTIFIER_VARIABLES)) +) + + +@pytest.fixture(scope="module") +def identifier_readers(tax_benefit_system) -> list: + """The variables whose formulas read an identifier.""" + readers = sorted( + name + for name, variable in tax_benefit_system.variables.items() + if any( + READS_AN_IDENTIFIER.search(inspect.getsource(formula)) + for formula in variable.formulas.values() + ) + ) + # The scan finds at least the readers known when it was written. + assert set(readers) >= { + "is_benunit_eldest_child", + "months_since_last_birthday", + "uc_deduction_random_draw", + "uc_deduction_type_random_draw", + } + return readers + + +def give_values_for( + simulation, population: Population, year: int, weight: int = 1 +) -> None: + """Set each of ``population``'s values on ``simulation`` for ``year``, with + household weights ``weight`` times theirs. + + A weight of a million makes the benefit units of any household of weight + one or more add up to the weight of representative data, for which the + Universal Credit deduction draws read the benefit unit identifiers. + """ + for variable, entity in VARIABLES.items(): + by_id = population.by_id(variable) + values = np.array([by_id[id_] for id_ in ids(simulation, entity)]) + if variable == "household_weight": + values = values * weight + simulation.set_input(variable, year, values) + + +# Years before the data year, the data year and years after it. +YEARS_AROUND = list(range(YEAR - 3, YEAR + 3)) +# Two children of one age in a benefit unit, in a household of weight one. +# The tie on age falls to their identifiers (is_benunit_eldest_child), the +# draw that places each in their year of age reads them +# (months_since_last_birthday), and at a million times the weight so do the +# Universal Credit deduction draws. Before simulations held their identifiers +# in a year before their data, each of the four differed there between a +# simulation and the one its export loads back to. +TWINS = Population( + person_ids=np.array([1, 2]), + groups={"benunit": np.array([1, 1]), "household": np.array([1, 1])}, + values={ + "age": np.array([10, 10]), + "hours_worked": np.array([0, 0]), + "gender": np.array(["MALE", "MALE"]), + "is_disabled_for_benefits": np.array([False, False]), + "benunit_reported_capital": np.array([0, 0]), + "would_claim_uc": np.array([False, False]), + "council_tax": np.array([0, 0]), + "household_weight": np.array([1, 1]), + "region": np.array(["LONDON", "LONDON"]), + }, +) + + +@ROUND_TRIP_SETTINGS +@given( + population=populations(), + id_columns=id_columns, + input_years=st.sets(st.sampled_from(YEARS_AROUND), min_size=1, max_size=2), + weight=st.sampled_from([1, 1_000_000]), +) +# The smallest example of the identifiers differing: one person's draw for +# their place in their year of age, in a year before the data. +@example(population=ONE_PERSON, id_columns="both", input_years={YEAR - 3}, weight=1) +@example(population=TWINS, id_columns="own", input_years={YEAR - 1}, weight=1_000_000) +def test_a_reloaded_simulation_reads_the_same_identifiers_in_each_year_given_inputs( + identifier_readers, population, id_columns, input_years, weight +): + # 10. Identifiers in each year: a simulation built from a frame and given + # values for other years, before the data year or after it, and the + # simulation its export loads back to compute the same values from the + # identifiers in each of those years. Both hold the people and groups' + # own identifiers there. + simulation = Simulation(dataset=frame(population, id_columns)) + for year in sorted(input_years): + give_values_for(simulation, population, year, weight) + exported = simulation.to_input_dataframe() + + reloaded = Simulation(dataset=exported) + + for year in sorted(input_years): + for variable in identifier_readers: + np.testing.assert_array_equal( + reloaded.calculate(variable, year), + simulation.calculate(variable, year), + err_msg=f"{variable} for {year}", + ) + for built in (simulation, reloaded): + people = built.calculate("person_id", year) + assert people.tolist() == population.person_ids.tolist() + for group in GROUPS: + assert built.calculate(f"{group}_id", year).tolist() == ids( + built, group + ) + membership = built.calculate(f"person_{group}_id", year) + exact = population.groups[group].astype(np.float32) + assert membership.tolist() == exact.tolist() + pd.testing.assert_frame_equal(reloaded.to_input_dataframe(), exported) + + +def identifier_inputs(simulation, branch: str = "default") -> dict: + """The periods each identifier is held for as an input, by identifier.""" + return { + identifier: sorted( + str(period) + for period in simulation.get_holder(identifier).get_input_periods(branch) + ) + for identifier in sorted(IDENTIFIER_VARIABLES) + } + + +def test_an_input_before_the_data_gives_the_identifiers_for_its_year(): + simulation = Simulation(dataset=frame(TWINS)) + data_years = [str(YEAR)] + assert set(map(tuple, identifier_inputs(simulation).values())) == { + tuple(data_years) + } + + # A month two years before the data: the identifiers are given for its + # year, and carry forward from there to the next. + simulation.set_input( + "ebr_energy_bills_credit_eligible", f"{YEAR - 2}-01", np.array([True]) + ) + + for periods in identifier_inputs(simulation).values(): + assert periods == [str(YEAR - 2), *data_years] + for year in (YEAR - 2, YEAR - 1): + assert simulation.calculate("person_id", year).tolist() == [1, 2] + assert simulation.calculate("benunit_id", year).tolist() == [1] + assert simulation.calculate("person_household_id", year).tolist() == [1, 1] + # Before that year the simulation holds no input, nor any identifier. + assert simulation.calculate("person_id", YEAR - 3).tolist() == [0, 0] + # An input for a later year gives none: they carry forward to it. + simulation.set_input("hours_worked", YEAR + 7, np.array([1, 2])) + for periods in identifier_inputs(simulation).values(): + assert periods == [str(YEAR - 2), *data_years] + assert simulation.calculate("person_id", YEAR + 7).tolist() == [1, 2] + # The month and the identifiers for its year load back as given. + exported = simulation.to_input_dataframe() + assert exported[f"person_id__{YEAR - 2}"].tolist() == [1, 2] + reloaded = Simulation(dataset=exported) + for periods in identifier_inputs(reloaded).values(): + assert periods[0] == str(YEAR - 2) + assert reloaded.calculate("person_id", YEAR - 1).tolist() == [1, 2] + pd.testing.assert_frame_equal(reloaded.to_input_dataframe(), exported) + + +def test_an_identifier_given_alone_before_the_data_brings_the_others(): + # One identifier for an earlier year, as a frame can name for a year it + # gives no values for: the person's groups are given for it too. + simulation = Simulation(dataset=frame(TWINS)) + simulation.set_input("person_id", YEAR - 1, np.array([1, 2])) + + for periods in identifier_inputs(simulation).values(): + assert periods[0] == str(YEAR - 1) + assert simulation.calculate("household_id", YEAR - 1).tolist() == [1] + assert simulation.calculate("person_benunit_id", YEAR - 1).tolist() == [1, 1] + + +def test_a_branch_given_an_input_before_the_data_holds_the_identifiers_itself(): + simulation = Simulation(dataset=frame(TWINS)) + branch = simulation.get_branch("earlier") + branch.set_input("hours_worked", YEAR - 1, np.array([10, 20])) + + assert branch.calculate("person_id", YEAR - 1).tolist() == [1, 2] + assert identifier_inputs(branch, "earlier")["person_id"][0] == str(YEAR - 1) + # The branch it was made from holds no input for that year, nor any + # identifier. + assert identifier_inputs(simulation)["person_id"][0] == str(YEAR) + assert simulation.calculate("person_id", YEAR - 1).tolist() == [0, 0] + + +def test_a_simulation_that_holds_no_identifiers_is_given_none(): + # A household situation names its people and groups but sets no + # identifier as an input, so an input for another year gives none. + simulation = Simulation( + situation={ + "people": {"adult": {"age": {YEAR: 40}}}, + "benunits": {"benunit": {"members": ["adult"]}}, + "households": {"household": {"members": ["adult"]}}, + } + ) + simulation.set_input("hours_worked", YEAR - 1, np.array([10])) + + assert identifier_inputs(simulation) == { + identifier: [] for identifier in sorted(IDENTIFIER_VARIABLES) + } + + +@pytest.mark.parametrize("descending", [False, True]) +def test_a_dataset_gives_the_identifiers_for_its_own_years_only(descending): + # As H5 files load: the identifiers are inputs in each year of the + # dataset, and loading gives them for no other year, in whichever order + # the dataset holds its years. + built = Simulation( + dataset=multi_year_frame(TWINS, "both", {YEAR}, {YEAR, YEAR + 2}) + ).dataset + years = [YEAR + 2, YEAR] if descending else [YEAR, YEAR + 2] + dataset = UKMultiYearDataset(datasets=[built[year] for year in years]) + + simulation = Simulation(dataset=dataset) + + assert identifier_inputs(simulation) == { + identifier: [str(YEAR), str(YEAR + 2)] + for identifier in sorted(IDENTIFIER_VARIABLES) + } + for year in (YEAR, YEAR + 2): + assert simulation.calculate("person_id", year).tolist() == [1, 2] + + @TABLE_SETTINGS @given(population=populations(), id_columns=id_columns, data=st.data()) def test_tables_hold_the_frame_in_any_row_order( From c48f3a9a51ae14b30d7b514e58b8e7f2b837290d Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 16:37:28 -0400 Subject: [PATCH 02/10] Give a simulation its identifiers for a year before its data policyengine-core carries an input forward but never back, so a simulation given an input for a year before every year it holds its identifiers for held none there: every person, benefit unit and household identifier read as zero. The frame it exports loads as a dataset that holds the people and groups in that year, so the reloaded simulation read the real identifiers there, and is_benunit_eldest_child, months_since_last_birthday and the Universal Credit deduction draws gave different values. Simulation.set_input now calls hold_identifiers_in, which gives each identifier the simulation holds as an input for some period, but for none starting by the first day of the year the input starts in, for that year, copied from the period it is held for that starts first. From there it carries forward. An input in or after the first year the identifiers are held changes nothing, and a simulation that holds no identifier as an input, such as one built from a situation, is left alone. Datasets load each year with its identifiers, so loading one gives none for any other year. The change to simulation.py is the set_input override and one import. Fixes #2353. Co-Authored-By: Claude Opus 5.5 --- changelog.d/2353.fixed.md | 1 + docs/book/usage/simulations.md | 6 +++ policyengine_uk/data/input_dataframe.py | 57 ++++++++++++++++++++++--- policyengine_uk/simulation.py | 9 ++++ 4 files changed, 66 insertions(+), 7 deletions(-) create mode 100644 changelog.d/2353.fixed.md diff --git a/changelog.d/2353.fixed.md b/changelog.d/2353.fixed.md new file mode 100644 index 0000000000..9a11406c91 --- /dev/null +++ b/changelog.d/2353.fixed.md @@ -0,0 +1 @@ +- Fixed a simulation given an input for a year before its data holding no person, benefit unit or household identifiers in that year, so that formulas reading them, such as `months_since_last_birthday` and the Universal Credit deduction draws, gave different values there from the simulation its exported frame loads back to. diff --git a/docs/book/usage/simulations.md b/docs/book/usage/simulations.md index 7b54a694da..2288feb70c 100644 --- a/docs/book/usage/simulations.md +++ b/docs/book/usage/simulations.md @@ -207,6 +207,12 @@ print(sim.calculate("council_tax", 2025)) # One value per household simulation was given inputs for years outside its data, such as the Universal Credit health element status the rebalancing records for 2026 to 2029. +- Inputs carry forward to later years but not back to earlier ones. So when + `set_input` gives a simulation an input for a year before every year it + holds the identifiers for, it also gives the identifiers for that year. + Formulas that read identifiers, such as the draws that place each person in + their year of age, then read the same people and groups there as in the + simulation its exported frame loads back to. ```{important} The model stores `person_household_id` and `person_benunit_id` as 32-bit diff --git a/policyengine_uk/data/input_dataframe.py b/policyengine_uk/data/input_dataframe.py index 6ff5c077a4..9db2f9afc5 100644 --- a/policyengine_uk/data/input_dataframe.py +++ b/policyengine_uk/data/input_dataframe.py @@ -20,7 +20,11 @@ holds them all as inputs in each whole year a column names. ``Simulation.to_input_dataframe`` gives them for each such year too (``with_identifiers_in_each_year``), so that an exported frame loads back to -a simulation that exports it again. +a simulation that exports it again. Inputs carry forward but not back, so +``Simulation.set_input`` gives a simulation its identifiers for a year +before every year it holds them for when it is given an input there +(``hold_identifiers_in``). Formulas that read them then read the same people +and groups in that year as the simulation the frame loads back to. """ import warnings @@ -28,7 +32,7 @@ import numpy as np import pandas as pd -from policyengine_core.periods import period as period_ +from policyengine_core.periods import ETERNITY, period as period_ from policyengine_uk.data.dataset_schema import ( UKMultiYearDataset, @@ -440,6 +444,45 @@ def no_tables() -> Dict[str, Dict[str, np.ndarray]]: return UKMultiYearDataset(datasets=datasets), replayed, identifier_inputs +def hold_identifiers_in(simulation, period) -> None: + """Give ``simulation`` its identifiers for the year ``period`` starts in, + if they start later. + + policyengine-core carries an input forward to later periods but never + back (``Simulation._calculate``). So a simulation given an input for a + year before its data, such as 2023 for a frame of 2025, would hold no + identifiers there. Formulas would read every person, benefit unit and + household identifier there as zero, the default. Yet the simulation it + exports to holds them there, since the dataset a frame loads as holds the + people and groups in each year the frame gives values for + (``dataset_from_input_dataframe``). + + So each identifier that ``simulation`` holds as an input for some period, + but for none that starts by the first day of the year ``period`` starts + in, is set for that year to its value in the period it is held for that + starts first. A simulation holds one set of people and groups, so that + value names the same ones. From that year on the identifier carries + forward, so the simulation holds its identifiers in each year it holds an + input for. An identifier the simulation does not hold as an input, as in + one built from a situation, is left alone. + """ + period = period_(period) + if period.unit == ETERNITY: + return + year = period_(str(period.start.year)) + branch = simulation.branch_name + for identifier in sorted(IDENTIFIER_VARIABLES): + holder = simulation.get_holder(identifier) + held = holder.get_input_periods(branch) + if not held: + continue + first = min(held, key=lambda held_period: held_period.start) + if first.start > year.start: + simulation.set_input( + identifier, year, np.array(holder.get_array(first, branch)) + ) + + def with_identifiers_in_each_year( df: pd.DataFrame, tax_benefit_system: "TaxBenefitSystem" ) -> pd.DataFrame: @@ -456,11 +499,11 @@ def with_identifiers_in_each_year( for a year after its data once it was built, as the Universal Credit rebalancing records each benefit unit's health element status for 2026 to 2029, holds its identifiers in that year only by carrying them forward - from an earlier one; and in a year before its data, where inputs do not - carry back, its identifiers are the default, zero. policyengine-core - exports an input only for the periods it was set for. Giving every - identifier for each year makes the frame load back to a simulation that - exports the same frame. + from an earlier one. (In a year before its data, where inputs do not + carry back, ``hold_identifiers_in`` gives it them as inputs.) + policyengine-core exports an input only for the periods it was set for. + Giving every identifier for each year makes the frame load back to a + simulation that exports the same frame. A simulation holds one set of people and groups, so every column of an identifier names the same ones (the loader refuses a frame whose columns diff --git a/policyengine_uk/simulation.py b/policyengine_uk/simulation.py index 26bbbe18f7..7a79a6b43e 100644 --- a/policyengine_uk/simulation.py +++ b/policyengine_uk/simulation.py @@ -34,6 +34,7 @@ from policyengine_uk.data.dataset_sources import materialize_gcs_dataset_url from policyengine_uk.data.input_dataframe import ( dataset_from_input_dataframe, + hold_identifiers_in, with_identifiers_in_each_year, ) from policyengine_uk.utils.dependencies import get_variable_dependencies @@ -452,6 +453,14 @@ def to_input_dataframe( self.tax_benefit_system, ) + def set_input(self, variable_name: str, period, value) -> None: + """Set an input as policyengine-core does, then give the simulation + its identifiers for the year the input starts in if it holds them + only for later years, since inputs do not carry back. See + ``policyengine_uk.data.input_dataframe.hold_identifiers_in``.""" + super().set_input(variable_name, period, value) + hold_identifiers_in(self, period) + def build_from_dataset(self, dataset: Dataset) -> None: """Build simulation from a Dataset object. From 38bfb0d6d88af820985ecea7deee646b7d28e9b2 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 16:41:30 -0400 Subject: [PATCH 03/10] Say which simulation holds the identifiers in hold_identifiers_in's docstring Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/data/input_dataframe.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/policyengine_uk/data/input_dataframe.py b/policyengine_uk/data/input_dataframe.py index 9db2f9afc5..d0a2cf8a45 100644 --- a/policyengine_uk/data/input_dataframe.py +++ b/policyengine_uk/data/input_dataframe.py @@ -452,10 +452,10 @@ def hold_identifiers_in(simulation, period) -> None: back (``Simulation._calculate``). So a simulation given an input for a year before its data, such as 2023 for a frame of 2025, would hold no identifiers there. Formulas would read every person, benefit unit and - household identifier there as zero, the default. Yet the simulation it - exports to holds them there, since the dataset a frame loads as holds the - people and groups in each year the frame gives values for - (``dataset_from_input_dataframe``). + household identifier there as zero, the default. Yet the simulation its + exported frame loads back to holds them there, since the dataset a frame + loads as holds the people and groups in each year the frame gives values + for (``dataset_from_input_dataframe``). So each identifier that ``simulation`` holds as an input for some period, but for none that starts by the first day of the year ``period`` starts From 4ca06690cacfe0206b01b42f76dbde8c28eeb3dc Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 16:58:44 -0400 Subject: [PATCH 04/10] Fold in #2349's review nits and place set_input beside delete_arrays From the review of #2349 (the export helper this builds on): - Limit the claims that the columns sort by period, and that the frame loads back to the same frame, to the default export. With include_computed_variables=True, core exports each variable's periods in the order they are stored. - Say that with_identifiers_in_each_year returns the frame itself when it adds nothing, and otherwise a new frame. - Test that the loader and the export find the same years for frames that name identifiers, values and months for different years. The round trip relies on their agreeing, and each works the years out separately. - Test that an export whose person_id differs between years, before or after the data, is still refused when loaded. #2076 also overrides Simulation.set_input, just after delete_arrays. Put this override in the same place, so that if both PRs land git reports a conflict to resolve, rather than one override silently replacing the other in the class body. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/data/input_dataframe.py | 12 ++-- policyengine_uk/simulation.py | 21 +++---- .../tests/test_dataframe_dataset.py | 55 +++++++++++++++++++ 3 files changed, 74 insertions(+), 14 deletions(-) diff --git a/policyengine_uk/data/input_dataframe.py b/policyengine_uk/data/input_dataframe.py index d0a2cf8a45..3ca70e9959 100644 --- a/policyengine_uk/data/input_dataframe.py +++ b/policyengine_uk/data/input_dataframe.py @@ -509,13 +509,17 @@ def with_identifiers_in_each_year( identifier names the same ones (the loader refuses a frame whose columns differ). An identifier added for a year copies its column for the latest year before it the frame gives, or else the earliest year it gives. Each - goes beside the identifier's other columns, in order of period, which is - where the default export (sorted by period) puts an input set for that - year. + goes beside the identifier's other columns, before the first whose period + sorts after its own. The default export sorts each variable's periods, so + that is where it puts an input set for that year. (With + ``include_computed_variables=True``, policyengine-core exports each + variable's periods in the order they are stored, and the frame is not + expected to load back to the same frame.) Only columns that name a variable of ``tax_benefit_system`` count, and an identifier the frame does not give for any year is not added. ``df`` is - not changed. + not changed: if every identifier it gives is given for each such year, + ``df`` itself is returned, and otherwise a new frame. """ named: Dict[str, Dict[int, object]] = {} years = set() diff --git a/policyengine_uk/simulation.py b/policyengine_uk/simulation.py index 7a79a6b43e..5571a02825 100644 --- a/policyengine_uk/simulation.py +++ b/policyengine_uk/simulation.py @@ -251,6 +251,14 @@ def delete_arrays(self, variable: str, period: Period = None) -> None: # and the current branch, ancestor branches and default branch. drop_missing_supplied_inputs(self, variable) + def set_input(self, variable_name: str, period, value) -> None: + """Set an input as policyengine-core does, then give the simulation + its identifiers for the year the input starts in if it holds them + only for later years, since inputs do not carry back. See + ``policyengine_uk.data.input_dataframe.hold_identifiers_in``.""" + super().set_input(variable_name, period, value) + hold_identifiers_in(self, period) + def reset_calculations(self): for variable in self.tax_benefit_system.variables: if variable not in self.input_variables: @@ -438,8 +446,9 @@ def to_input_dataframe( policyengine-core exports each input for the periods it was set for. The identifiers are also given for every whole year a column names, - where a simulation built from the frame holds them, so that the frame - loads back to a simulation that exports the same frame. See + where a simulation built from the frame holds them, so that the + default export loads back to a simulation that exports the same + frame. See ``policyengine_uk.data.input_dataframe.with_identifiers_in_each_year``. Args: @@ -453,14 +462,6 @@ def to_input_dataframe( self.tax_benefit_system, ) - def set_input(self, variable_name: str, period, value) -> None: - """Set an input as policyengine-core does, then give the simulation - its identifiers for the year the input starts in if it holds them - only for later years, since inputs do not carry back. See - ``policyengine_uk.data.input_dataframe.hold_identifiers_in``.""" - super().set_input(variable_name, period, value) - hold_identifiers_in(self, period) - def build_from_dataset(self, dataset: Dataset) -> None: """Build simulation from a Dataset object. diff --git a/policyengine_uk/tests/test_dataframe_dataset.py b/policyengine_uk/tests/test_dataframe_dataset.py index abd4e8b301..549c604bae 100644 --- a/policyengine_uk/tests/test_dataframe_dataset.py +++ b/policyengine_uk/tests/test_dataframe_dataset.py @@ -1633,6 +1633,61 @@ def test_a_dataset_gives_the_identifiers_for_its_own_years_only(descending): assert simulation.calculate("person_id", year).tolist() == [1, 2] +@TABLE_SETTINGS +@given( + population=populations(), + id_columns=id_columns, + identifier_years=st.sets(st.sampled_from(YEARS), min_size=1, max_size=2), + data_years=st.sets(st.sampled_from(YEARS), max_size=3), + month_years=st.sets(st.sampled_from(YEARS), max_size=1), +) +def test_the_export_and_the_loader_find_the_same_years( + tax_benefit_system, + population, + id_columns, + identifier_years, + data_years, + month_years, +): + # The export gives each identifier for the years a simulation built from + # the frame holds it in: the dataset's years and the years only an + # identifier column names. The loader and the export each work these out + # from the frame's columns, so the round trip relies on them agreeing. + # Months name no year in either. + frame_ = multi_year_frame(population, id_columns, identifier_years, data_years) + for year in sorted(month_years): + frame_[f"ebr_energy_bills_credit_eligible__{year}-01"] = True + + dataset, _, identifier_inputs = dataset_from_input_dataframe( + frame_, tax_benefit_system, YEAR + ) + exported = with_identifiers_in_each_year(frame_, tax_benefit_system) + + held = {int(year) for year in dataset.years} | { + int(period) for _, period, _ in identifier_inputs + } + assert held == set(identifier_years) | set(data_years) + named = {split_column(column)[0] for column in frame_} + for identifier in IDENTIFIER_VARIABLES: + given_for = { + int(split_column(column)[1]) for column in columns_of(exported, identifier) + } + assert given_for == (held if identifier in named else set()) + + +@pytest.mark.parametrize("year", [YEAR - 2, YEAR + 2]) +def test_an_export_whose_identifiers_differ_between_years_is_refused(year): + # The export copies an identifier into the years it does not give it + # for; that never makes columns that differ agree, so the loader still + # refuses them. + simulation = Simulation(dataset=frame(TWINS)) + simulation.set_input("person_id", year, np.array([1, 3])) + exported = simulation.to_input_dataframe() + + with pytest.raises(ValueError, match="differ"): + Simulation(dataset=exported) + + @TABLE_SETTINGS @given(population=populations(), id_columns=id_columns, data=st.data()) def test_tables_hold_the_frame_in_any_row_order( From 1d5c9953f43f64a167450b6916f937a63c4fe9e2 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 17:00:53 -0400 Subject: [PATCH 05/10] Give the years test a lone person_id column, which names a year for every identifier Without it, every identifier was named for the same years, so the test passed even with an export that ignored years only an identifier names. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/tests/test_dataframe_dataset.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/policyengine_uk/tests/test_dataframe_dataset.py b/policyengine_uk/tests/test_dataframe_dataset.py index 549c604bae..d7857bbf0f 100644 --- a/policyengine_uk/tests/test_dataframe_dataset.py +++ b/policyengine_uk/tests/test_dataframe_dataset.py @@ -1639,6 +1639,7 @@ def test_a_dataset_gives_the_identifiers_for_its_own_years_only(descending): id_columns=id_columns, identifier_years=st.sets(st.sampled_from(YEARS), min_size=1, max_size=2), data_years=st.sets(st.sampled_from(YEARS), max_size=3), + lone_years=st.sets(st.sampled_from(YEARS), max_size=1), month_years=st.sets(st.sampled_from(YEARS), max_size=1), ) def test_the_export_and_the_loader_find_the_same_years( @@ -1647,14 +1648,18 @@ def test_the_export_and_the_loader_find_the_same_years( id_columns, identifier_years, data_years, + lone_years, month_years, ): # The export gives each identifier for the years a simulation built from # the frame holds it in: the dataset's years and the years only an # identifier column names. The loader and the export each work these out # from the frame's columns, so the round trip relies on them agreeing. - # Months name no year in either. + # A person_id column alone names a year for every identifier; months + # name no year in either. frame_ = multi_year_frame(population, id_columns, identifier_years, data_years) + for year in sorted(lone_years): + frame_[f"person_id__{year}"] = population.person_ids for year in sorted(month_years): frame_[f"ebr_energy_bills_credit_eligible__{year}-01"] = True @@ -1666,7 +1671,7 @@ def test_the_export_and_the_loader_find_the_same_years( held = {int(year) for year in dataset.years} | { int(period) for _, period, _ in identifier_inputs } - assert held == set(identifier_years) | set(data_years) + assert held == set(identifier_years) | set(data_years) | set(lone_years) named = {split_column(column)[0] for column in frame_} for identifier in IDENTIFIER_VARIABLES: given_for = { From c10e7e3a1f23e062c314cd85f69abbf745621878 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 17:01:57 -0400 Subject: [PATCH 06/10] Test that each method of the simulation classes is defined once The previous commit put this PR's set_input override beside delete_arrays expecting git to conflict with #2076's override there. It does not: git merges the two (`git merge-tree` exits 0), and the class then defines set_input twice. The later definition, this PR's, replaces #2076's, so its forget_model_value call would be lost without an error. CI checks formatting only, not `ruff check`, whose F811 reports this. This code-health test fails when Simulation or Microsimulation defines a method more than once (property setters, deleters and overload stubs aside). It fails on the tree merged with #2076, so whichever of the two lands second must combine the overrides into one: call core's set_input, then forget_model_value, then hold_identifiers_in. Co-Authored-By: Claude Opus 5.5 --- .../test_simulation_methods_defined_once.py | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 policyengine_uk/tests/code_health/test_simulation_methods_defined_once.py diff --git a/policyengine_uk/tests/code_health/test_simulation_methods_defined_once.py b/policyengine_uk/tests/code_health/test_simulation_methods_defined_once.py new file mode 100644 index 0000000000..3984a79b55 --- /dev/null +++ b/policyengine_uk/tests/code_health/test_simulation_methods_defined_once.py @@ -0,0 +1,76 @@ +"""Each method of the simulation classes is defined once. + +Pull requests often override methods of ``policyengine_uk.simulation.Simulation`` +(``set_input``, ``delete_arrays``, ``clone`` and others). Two overrides of one +method, added at different places in the class, merge without a git conflict, +and the later definition replaces the earlier one: whatever the earlier one +did is lost without an error. Combine them into one method instead. + +``ruff check`` reports this as F811, but CI checks formatting only, so this +test does. Property setters and deleters, and ``typing.overload`` stubs, name +a method again on purpose and are left out. +""" + +import ast +from collections import Counter +from pathlib import Path + +import pytest + +PACKAGE = Path(__file__).resolve().parents[2] +MODULES = ["simulation.py", "microsimulation.py"] + + +def _names_again(decorator: ast.expr) -> bool: + """Whether ``decorator`` defines a method under a name already used.""" + if isinstance(decorator, ast.Attribute): + return decorator.attr in ("setter", "deleter", "getter") + return isinstance(decorator, ast.Name) and decorator.id == "overload" + + +def methods_defined_more_than_once(source: str) -> dict: + """The methods each class in ``source`` defines more than once, by + class.""" + repeated = {} + for node in ast.walk(ast.parse(source)): + if not isinstance(node, ast.ClassDef): + continue + names = Counter( + method.name + for method in node.body + if isinstance(method, (ast.FunctionDef, ast.AsyncFunctionDef)) + and not any(map(_names_again, method.decorator_list)) + ) + twice = sorted(name for name, count in names.items() if count > 1) + if twice: + repeated[node.name] = twice + return repeated + + +@pytest.mark.parametrize("module", MODULES) +def test_each_method_of_the_simulation_classes_is_defined_once(module): + repeated = methods_defined_more_than_once((PACKAGE / module).read_text()) + assert not repeated, ( + f"{module} defines these methods more than once, so only the last " + f"definition of each takes effect: {repeated}. Combine them." + ) + + +def test_a_method_defined_twice_is_found(): + source = """ +class Simulation: + def set_input(self): + pass + + @property + def size(self): + pass + + @size.setter + def size(self, value): + pass + + def set_input(self): + pass +""" + assert methods_defined_more_than_once(source) == {"Simulation": ["set_input"]} From d2f48c86f9440a131b80730bdd2aaf359b5616d9 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 17:10:07 -0400 Subject: [PATCH 07/10] Give no identifiers for an input core ignores, nor ones it would ignore From the review of 38bfb0d6d: policyengine-core's set_input stores nothing for a period after the variable ends, but the override still gave the identifiers for that year. If an identifier itself had ended, setting it would store nothing, and hold_identifiers_in would call set_input again without end. hold_identifiers_in now takes the input's variable and returns when core ignores the input, and skips an identifier core would ignore for the year. No variable in policyengine-uk has an end date today, so nothing changes now; the tests give one for a single test. Also corrects its docstring: the reloaded simulation holds the people and groups in a year when the exported frame gives a whole year's value for it, not in each year the frame gives values for. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/data/input_dataframe.py | 31 +++++++++---- policyengine_uk/simulation.py | 2 +- .../tests/test_dataframe_dataset.py | 43 +++++++++++++++++++ 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/policyengine_uk/data/input_dataframe.py b/policyengine_uk/data/input_dataframe.py index 3ca70e9959..7204879638 100644 --- a/policyengine_uk/data/input_dataframe.py +++ b/policyengine_uk/data/input_dataframe.py @@ -444,18 +444,26 @@ def no_tables() -> Dict[str, Dict[str, np.ndarray]]: return UKMultiYearDataset(datasets=datasets), replayed, identifier_inputs -def hold_identifiers_in(simulation, period) -> None: - """Give ``simulation`` its identifiers for the year ``period`` starts in, - if they start later. +def _ignored(variable, period) -> bool: + """Whether policyengine-core's ``Simulation.set_input`` stores nothing + for ``variable`` in ``period``, as it does for a period that starts after + the variable ends.""" + return variable.end is not None and period.start.date > variable.end + + +def hold_identifiers_in(simulation, variable_name: str, period) -> None: + """Give ``simulation`` its identifiers for the year an input of + ``variable_name`` for ``period`` starts in, if they start later. policyengine-core carries an input forward to later periods but never back (``Simulation._calculate``). So a simulation given an input for a year before its data, such as 2023 for a frame of 2025, would hold no identifiers there. Formulas would read every person, benefit unit and - household identifier there as zero, the default. Yet the simulation its - exported frame loads back to holds them there, since the dataset a frame - loads as holds the people and groups in each year the frame gives values - for (``dataset_from_input_dataframe``). + household identifier there as zero, the default. Yet once the frame it + exports gives a whole year's value for that year, the frame loads back to + a simulation that holds the people and groups there: each whole year a + column names is a year of the dataset, which holds them + (``dataset_from_input_dataframe``). So each identifier that ``simulation`` holds as an input for some period, but for none that starts by the first day of the year ``period`` starts @@ -464,14 +472,19 @@ def hold_identifiers_in(simulation, period) -> None: value names the same ones. From that year on the identifier carries forward, so the simulation holds its identifiers in each year it holds an input for. An identifier the simulation does not hold as an input, as in - one built from a situation, is left alone. + one built from a situation, is left alone. So is an input core does not + store, for a period after its variable ends, and an identifier core would + not store for that year. """ period = period_(period) - if period.unit == ETERNITY: + variables = simulation.tax_benefit_system.variables + if period.unit == ETERNITY or _ignored(variables[variable_name], period): return year = period_(str(period.start.year)) branch = simulation.branch_name for identifier in sorted(IDENTIFIER_VARIABLES): + if _ignored(variables[identifier], year): + continue holder = simulation.get_holder(identifier) held = holder.get_input_periods(branch) if not held: diff --git a/policyengine_uk/simulation.py b/policyengine_uk/simulation.py index 5571a02825..5f957dca78 100644 --- a/policyengine_uk/simulation.py +++ b/policyengine_uk/simulation.py @@ -257,7 +257,7 @@ def set_input(self, variable_name: str, period, value) -> None: only for later years, since inputs do not carry back. See ``policyengine_uk.data.input_dataframe.hold_identifiers_in``.""" super().set_input(variable_name, period, value) - hold_identifiers_in(self, period) + hold_identifiers_in(self, variable_name, period) def reset_calculations(self): for variable in self.tax_benefit_system.variables: diff --git a/policyengine_uk/tests/test_dataframe_dataset.py b/policyengine_uk/tests/test_dataframe_dataset.py index d7857bbf0f..4dc1702ced 100644 --- a/policyengine_uk/tests/test_dataframe_dataset.py +++ b/policyengine_uk/tests/test_dataframe_dataset.py @@ -66,6 +66,7 @@ anything, since it also exports every calculated value. """ +import datetime import inspect import io import re @@ -1595,6 +1596,48 @@ def test_a_branch_given_an_input_before_the_data_holds_the_identifiers_itself(): assert simulation.calculate("person_id", YEAR - 1).tolist() == [0, 0] +@pytest.fixture +def ending(): + """Give a variable of a simulation an end date for one test, then take + it away: ``ending(simulation, variable, year)`` ends it on the last day + of ``year``.""" + ended = [] + + def end(simulation, name: str, year: int) -> None: + variable = simulation.tax_benefit_system.get_variable(name) + ended.append((variable, variable.end)) + variable.end = datetime.date(year, 12, 31) + + yield end + for variable, end in ended: + variable.end = end + + +def test_an_input_core_stores_nothing_for_gives_no_identifiers(ending): + # policyengine-core ignores an input for a period after its variable + # ends, so the simulation holds no input in that year. + simulation = Simulation(dataset=frame(TWINS)) + ending(simulation, "hours_worked", YEAR - 3) + + simulation.set_input("hours_worked", YEAR - 2, np.array([1, 2])) + + assert set(map(tuple, identifier_inputs(simulation).values())) == {(str(YEAR),)} + + +def test_an_identifier_core_would_not_store_is_left_out(ending): + # Setting it would store nothing, and hold_identifiers_in would try + # again without end. + simulation = Simulation(dataset=frame(TWINS)) + ending(simulation, "household_id", YEAR - 3) + + simulation.set_input("hours_worked", YEAR - 2, np.array([1, 2])) + + held = identifier_inputs(simulation) + assert held.pop("household_id") == [str(YEAR)] + for periods in held.values(): + assert periods == [str(YEAR - 2), str(YEAR)] + + def test_a_simulation_that_holds_no_identifiers_is_given_none(): # A household situation names its people and groups but sets no # identifier as an input, so an input for another year gives none. From f9ddc20a217fda1f2399623700caf31dae96f3ec Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 17:13:26 -0400 Subject: [PATCH 08/10] Say the methods-defined-once test guards the class wherever lint is not run #2355 adds ruff check --select F811,F821 to Lint, so 'CI checks formatting only' would go stale once it lands. Co-Authored-By: Claude Opus 5.5 --- .../code_health/test_simulation_methods_defined_once.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/policyengine_uk/tests/code_health/test_simulation_methods_defined_once.py b/policyengine_uk/tests/code_health/test_simulation_methods_defined_once.py index 3984a79b55..a5ad69bc84 100644 --- a/policyengine_uk/tests/code_health/test_simulation_methods_defined_once.py +++ b/policyengine_uk/tests/code_health/test_simulation_methods_defined_once.py @@ -6,9 +6,10 @@ and the later definition replaces the earlier one: whatever the earlier one did is lost without an error. Combine them into one method instead. -``ruff check`` reports this as F811, but CI checks formatting only, so this -test does. Property setters and deleters, and ``typing.overload`` stubs, name -a method again on purpose and are left out. +``ruff check`` reports this as F811 when it runs. This test guards the merged +class with the tests themselves, wherever lint is not run. Property setters +and deleters, and ``typing.overload`` stubs, name a method again on purpose +and are left out. """ import ast From 11b8067329600dcb76f45e2d85fc8a9b2e41e7fe Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 18:52:57 -0400 Subject: [PATCH 09/10] Drop the redundant identifier check and correct what stops the recursion From the review of f9ddc20a2: - The check that skipped an identifier core would not store did nothing. hold_identifiers_in sets each identifier through simulation.set_input, which calls it again for that identifier, and the check that an input core ignores gives no identifiers already returns there. With the skipped check deleted both tests still pass; with the remaining check removed as well, the ended-identifier case raises RecursionError (both executed). So the last commit's claim, and the test's comment, that the deleted check prevented the recursion were wrong. The docstring now says how the calls end. - The docstring said each whole year a column names is a year of the dataset. A year only an identifier column names is not: the loader sets the identifiers there as inputs. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/data/input_dataframe.py | 16 +++++++++------- policyengine_uk/tests/test_dataframe_dataset.py | 5 +++-- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/policyengine_uk/data/input_dataframe.py b/policyengine_uk/data/input_dataframe.py index 7204879638..fae8c23a74 100644 --- a/policyengine_uk/data/input_dataframe.py +++ b/policyengine_uk/data/input_dataframe.py @@ -462,8 +462,9 @@ def hold_identifiers_in(simulation, variable_name: str, period) -> None: household identifier there as zero, the default. Yet once the frame it exports gives a whole year's value for that year, the frame loads back to a simulation that holds the people and groups there: each whole year a - column names is a year of the dataset, which holds them - (``dataset_from_input_dataframe``). + column of another variable names is a year of the dataset, which holds + them, and each other year an identifier column names gets the identifiers + as inputs (``dataset_from_input_dataframe``). So each identifier that ``simulation`` holds as an input for some period, but for none that starts by the first day of the year ``period`` starts @@ -472,9 +473,12 @@ def hold_identifiers_in(simulation, variable_name: str, period) -> None: value names the same ones. From that year on the identifier carries forward, so the simulation holds its identifiers in each year it holds an input for. An identifier the simulation does not hold as an input, as in - one built from a situation, is left alone. So is an input core does not - store, for a period after its variable ends, and an identifier core would - not store for that year. + one built from a situation, is left alone. + + An input core does not store, for a period after its variable ends, gives + none. Each identifier is set through ``simulation.set_input``, which + calls this again for the identifier. That call finds the identifier held + for the year, or returns here if core did not store it, so the calls end. """ period = period_(period) variables = simulation.tax_benefit_system.variables @@ -483,8 +487,6 @@ def hold_identifiers_in(simulation, variable_name: str, period) -> None: year = period_(str(period.start.year)) branch = simulation.branch_name for identifier in sorted(IDENTIFIER_VARIABLES): - if _ignored(variables[identifier], year): - continue holder = simulation.get_holder(identifier) held = holder.get_input_periods(branch) if not held: diff --git a/policyengine_uk/tests/test_dataframe_dataset.py b/policyengine_uk/tests/test_dataframe_dataset.py index 4dc1702ced..bcdaada276 100644 --- a/policyengine_uk/tests/test_dataframe_dataset.py +++ b/policyengine_uk/tests/test_dataframe_dataset.py @@ -1625,8 +1625,9 @@ def test_an_input_core_stores_nothing_for_gives_no_identifiers(ending): def test_an_identifier_core_would_not_store_is_left_out(ending): - # Setting it would store nothing, and hold_identifiers_in would try - # again without end. + # Core stores nothing for an identifier that has ended. The attempt to + # give it ends there, since an input core ignores gives no identifiers, + # and the other identifiers are still given. simulation = Simulation(dataset=frame(TWINS)) ending(simulation, "household_id", YEAR - 3) From 470de30403909d87f02fb2e4cc4d1b8647edb895 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 22 Oct 2026 14:47:36 -0400 Subject: [PATCH 10/10] Combine the two Simulation.set_input overrides Main gained a set_input override (model-value records and CGT input capture) after this branch's own (holding identifiers for the year an input starts in). The merge kept both definitions, and the later one silently replaced the earlier: ruff F811 in CI's Lint. Keep one override that does both: core's set_input, then forget replaced model values, then hold the identifiers, then capture CGT inputs. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/simulation.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/policyengine_uk/simulation.py b/policyengine_uk/simulation.py index 571f4f531f..de708fb402 100644 --- a/policyengine_uk/simulation.py +++ b/policyengine_uk/simulation.py @@ -571,6 +571,10 @@ def set_input(self, variable_name: str, period: Period, value) -> None: super().set_input(variable_name, period, value) finally: forget_replaced_model_values(self, variable_name, records) + # Inputs do not carry back, so give the simulation its identifiers + # for the year the input starts in if it holds them only for later + # years. See ``policyengine_uk.data.input_dataframe.hold_identifiers_in``. + hold_identifiers_in(self, variable_name, period) if variable_name in CGT_DATASET_INPUTS: self._capture_cgt_supplied_inputs(variable_name, replace_period=period) @@ -835,14 +839,6 @@ def _dataset_cache_key(self, source: str): metadata["policyengine_core_version"], ) - def set_input(self, variable_name: str, period, value) -> None: - """Set an input as policyengine-core does, then give the simulation - its identifiers for the year the input starts in if it holds them - only for later years, since inputs do not carry back. See - ``policyengine_uk.data.input_dataframe.hold_identifiers_in``.""" - super().set_input(variable_name, period, value) - hold_identifiers_in(self, variable_name, period) - def reset_calculations(self): for variable in self.tax_benefit_system.variables: if variable not in self.input_variables: