From f91065f4c6972a923c19d5f28de66c90fb80d7c2 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 12:12:39 -0400 Subject: [PATCH 01/12] Export identifiers for every year an input frame gives values for Simulation.to_input_dataframe() now gives the person, benefit unit and household identifiers for each whole year the frame gives other values for. The dataset a frame loads as holds them in each of those years, so a simulation built from the frame records them there and exported them, while the simulation that made the frame held them in a year after its data only by carrying them forward. Since #2251 every simulation records uc_receives_new_claimant_health_element for 2026 to 2029, so every frame export failed the round trip that #2270's property test checks: 18 columns exported, 38 re-exported. Adds the minimal counterexample as an explicit example, regression tests for the rebalancing's years and for inputs set by hand before, among and after them, a unit test of the helper, and a property that a frame over several years, given inputs for other years once built, exports the same frame again. Co-Authored-By: Claude Opus 5.5 --- .../fix-roundtrip-recorded-inputs.fixed.md | 1 + docs/book/usage/simulations.md | 6 + policyengine_uk/data/input_dataframe.py | 92 ++++++++- policyengine_uk/simulation.py | 28 ++- .../tests/test_dataframe_dataset.py | 193 +++++++++++++++++- 5 files changed, 313 insertions(+), 7 deletions(-) create mode 100644 changelog.d/fix-roundtrip-recorded-inputs.fixed.md diff --git a/changelog.d/fix-roundtrip-recorded-inputs.fixed.md b/changelog.d/fix-roundtrip-recorded-inputs.fixed.md new file mode 100644 index 0000000000..9d7e60fdcf --- /dev/null +++ b/changelog.d/fix-roundtrip-recorded-inputs.fixed.md @@ -0,0 +1 @@ +- Fixed `Simulation.to_input_dataframe()` leaving out the person, benefit unit and household identifiers for years in which a simulation was given inputs after it was built, such as the Universal Credit health element status for 2026 to 2029, so that an exported frame now loads back to a simulation that exports the same frame. diff --git a/docs/book/usage/simulations.md b/docs/book/usage/simulations.md index 4128daf55b..aa00d945df 100644 --- a/docs/book/usage/simulations.md +++ b/docs/book/usage/simulations.md @@ -200,6 +200,12 @@ print(sim.calculate("council_tax", 2025)) # One value per household period that is not one year are set on the simulation directly. A variable given for both months and the year is set column by column in the frame's order, since a year's value is spread over the months not already set. +- `to_input_dataframe()` gives the identifier columns for every year it gives + other values for, which is where the dataset the frame loads as holds them. + So an exported frame loads back to a simulation that exports the same frame, + even when the simulation was given inputs for years after its data, such as + the Universal Credit health element status the rebalancing records for 2026 + to 2029. ```{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 fcc6b133d1..60da596084 100644 --- a/policyengine_uk/data/input_dataframe.py +++ b/policyengine_uk/data/input_dataframe.py @@ -15,6 +15,11 @@ Identifiers are read as whole numbers, value by value, and a float that may stand for more than one whole number is refused rather than allowed to merge two people or groups. + +The identifiers hold for every period, and the dataset a frame loads as holds +them in each year the frame gives data for. ``Simulation.to_input_dataframe`` +writes them for each of those years too (``with_identifiers_in_each_year``), +so that an exported frame loads back to a simulation that exports it again. """ import warnings @@ -267,6 +272,17 @@ def _holds_floats(identifiers: pd.Series) -> bool: return values.dtype.kind == "f" +def _is_whole_year(period) -> bool: + """Whether ``period`` is one calendar year, the period of a dataset + year.""" + start = period.start + return ( + period.unit == "year" + and period.size == 1 + and (start.month, start.day) == (1, 1) + ) + + def _same(left: np.ndarray, right: np.ndarray) -> np.ndarray: """Whether each pair of values is equal, counting two missing values as equal.""" @@ -362,11 +378,7 @@ def no_tables() -> Dict[str, Dict[str, np.ndarray]]: continue period = period_(period_text) start = period.start - whole_year = ( - period.unit == "year" - and period.size == 1 - and (start.month, start.day) == (1, 1) - ) + whole_year = _is_whole_year(period) if variable in IDENTIFIER_VARIABLES: if whole_year: identifier_years.add(start.year) @@ -425,3 +437,73 @@ def no_tables() -> Dict[str, Dict[str, np.ndarray]]: for fiscal_year, tables in sorted(years.items()) ] return UKMultiYearDataset(datasets=datasets), replayed, identifier_inputs + + +def with_identifiers_in_each_year( + df: pd.DataFrame, tax_benefit_system: "TaxBenefitSystem" +) -> pd.DataFrame: + """``df``, an exported frame, with each identifier it gives for some year + given for every year it gives other values for. + + The dataset a frame loads as holds the people and groups in each year the + frame gives values for (``dataset_from_input_dataframe``), so a simulation + built from it holds the identifiers as inputs in those years, and exports + them there. A simulation that records an input for a later year once it + is 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, and policyengine-core exports only + the periods an input was set for. Giving the identifiers for that 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 + 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, where + policyengine-core exports an input set for that year. + + Years are those ``dataset_from_input_dataframe`` makes dataset years: the + whole calendar years that columns of other variables of + ``tax_benefit_system`` name. ``df`` is not changed. + """ + named: Dict[str, Dict[int, object]] = {} + years = set() + for column in df.columns: + variable, period_text = split_column(column) + if variable not in tax_benefit_system.variables: + continue + period = period_(period_text) + if not _is_whole_year(period): + continue + if variable in IDENTIFIER_VARIABLES: + named.setdefault(variable, {})[period.start.year] = column + else: + years.add(period.start.year) + added = { + variable: sorted(years - set(columns)) + for variable, columns in named.items() + if years - set(columns) + } + if not added: + return df + result = df.copy(deep=False) + for variable, missing in added.items(): + columns = named[variable] + for year in missing: + earlier = [named_year for named_year in columns if named_year < year] + source = columns[max(earlier) if earlier else min(columns)] + name = f"{variable}{COLUMN_SEPARATOR}{year}" + # The identifier's columns, by period text, as the exporter sorts + # them; the new one goes before the first that sorts after it. + own = [ + position + for position, column in enumerate(result.columns) + if split_column(column)[0] == variable + ] + later = [ + position + for position in own + if split_column(result.columns[position])[1] > str(year) + ] + result.insert(later[0] if later else own[-1] + 1, name, df[source].copy()) + return result diff --git a/policyengine_uk/simulation.py b/policyengine_uk/simulation.py index 2acef5aefe..a709ffd65d 100644 --- a/policyengine_uk/simulation.py +++ b/policyengine_uk/simulation.py @@ -32,7 +32,10 @@ reset_growthfactor_uprating, ) from policyengine_uk.data.dataset_sources import materialize_gcs_dataset_url -from policyengine_uk.data.input_dataframe import dataset_from_input_dataframe +from policyengine_uk.data.input_dataframe import ( + dataset_from_input_dataframe, + with_identifiers_in_each_year, +) from policyengine_uk.utils.dependencies import get_variable_dependencies from policyengine_uk.utils.supplied_inputs import ( SUPPLIED_INPUT_VARIABLES, @@ -426,6 +429,29 @@ def build_from_dataframe(self, df: pd.DataFrame) -> None: for variable, time_period, values in replayed: self.set_input(variable, time_period, values) + def to_input_dataframe( + self, include_computed_variables: bool = False + ) -> pd.DataFrame: + """Export the simulation's inputs as a DataFrame that + ``Simulation(dataset=...)`` loads back. + + policyengine-core exports each input for the periods it was set for. + The identifiers are also given for every year the frame gives other + values for, as the dataset the frame loads as holds them, so that the + frame loads back to a simulation that exports the same frame. See + ``policyengine_uk.data.input_dataframe.with_identifiers_in_each_year``. + + Args: + include_computed_variables: If True, export every stored value, + calculated ones included, as policyengine-core does. + """ + return with_identifiers_in_each_year( + super().to_input_dataframe( + include_computed_variables=include_computed_variables + ), + self.tax_benefit_system, + ) + 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 c3793a4f51..30d5e9b595 100644 --- a/policyengine_uk/tests/test_dataframe_dataset.py +++ b/policyengine_uk/tests/test_dataframe_dataset.py @@ -30,6 +30,12 @@ 5. Round trip: the frame a simulation exports loads back to the same people, groups and inputs, and exports the same frame again; so does that frame saved as CSV, which holds the exported 32-bit identifiers as 64-bit floats. + This holds for frames that name the identifiers and give values for + different years, and for simulations given inputs for other years once + built, as the Universal Credit rebalancing gives each benefit unit's + health element status for 2026 to 2029. The export gives the identifiers, + exactly, for every year it gives other values for, which is where the + dataset the frame loads as holds them. 6. Differential: a simulation built from a dataset and one built from the frame it exports give the same value for every variable of the model. 7. Exact names: a column is read only if the part of its name before ``__`` @@ -67,7 +73,7 @@ import numpy as np import pandas as pd import pytest -from hypothesis import HealthCheck, given, settings +from hypothesis import HealthCheck, example, given, settings from hypothesis import strategies as st from policyengine_uk import CountryTaxBenefitSystem, Microsimulation, Simulation @@ -77,6 +83,7 @@ IDENTIFIER_VARIABLES, dataset_from_input_dataframe, split_column, + with_identifiers_in_each_year, ) from policyengine_uk.scenarios import no_economic_assumptions @@ -601,6 +608,57 @@ def test_a_frame_that_names_no_whole_year_is_held_in_the_default_input_year(): ) == {1: True} +def test_an_export_gives_its_identifiers_for_each_year_it_gives_values_for( + tax_benefit_system, +): + # Values for 2024, 2025, 2027 and 2029 make those years of the dataset, + # which holds the identifiers in each. A month, a year only an identifier + # names, and a column that names no variable add none. Each added column + # copies the identifier's latest earlier column, or else its earliest: + # here person_id__2028 is given as floats, so person_id__2029 is too. + frame_ = pd.DataFrame( + { + "benunit_id__2025": [7, 7], + "person_id__2025": [1, 2], + "person_id__2028": [1.0, 2.0], + "age__2024": [39, 37], + "age__2025": [40, 38], + "age__2027": [42, 40], + "ebr_energy_bills_credit_eligible__2026-01": [True, True], + "council_tax__2029": [1_500.0, 1_500.0], + "previous_household_id__2030": [5, 5], + } + ) + before = frame_.copy() + + exported = with_identifiers_in_each_year(frame_, tax_benefit_system) + + assert exported.columns.tolist() == [ + "benunit_id__2024", + "benunit_id__2025", + "benunit_id__2027", + "benunit_id__2029", + "person_id__2024", + "person_id__2025", + "person_id__2027", + "person_id__2028", + "person_id__2029", + *frame_.columns[3:], + ] + for year, source in [(2024, 2025), (2027, 2025), (2029, 2028)]: + pd.testing.assert_series_equal( + exported[f"person_id__{year}"], + frame_[f"person_id__{source}"], + check_names=False, + ) + pd.testing.assert_frame_equal(exported[frame_.columns], frame_) + pd.testing.assert_frame_equal(frame_, before) + # A frame that gives its identifiers for each such year gains nothing. + pd.testing.assert_frame_equal( + with_identifiers_in_each_year(exported, tax_benefit_system), exported + ) + + # The dataset a frame-built simulation holds. @@ -1140,6 +1198,29 @@ def test_members_may_all_leave_a_group_value_missing(tax_benefit_system): suppress_health_check=[HealthCheck.too_slow, HealthCheck.data_too_large], ) ROUND_TRIP_SETTINGS = settings(SIMULATION_SETTINGS, max_examples=15) +# The smallest example of the round trip failing once the Universal Credit +# rebalancing recorded each benefit unit's health element status for 2026 to +# 2029 (#2251). The export gave that status for those years but the person +# and their groups for 2025 alone. Loaded back, the frame was a dataset with a +# year for each of 2025 to 2029, which holds the identifiers in each, so the +# second export gave them for 2026 to 2029 as well: 38 columns, not 18. +ONE_PERSON = Population( + person_ids=np.array([1]), + groups={"benunit": np.array([1]), "household": np.array([1])}, + values={ + "age": np.array([0]), + "hours_worked": np.array([0]), + "gender": np.array(["MALE"]), + "is_disabled_for_benefits": np.array([False]), + "benunit_reported_capital": np.array([0]), + "would_claim_uc": np.array([False]), + "council_tax": np.array([0]), + "household_weight": np.array([0]), + "region": np.array(["LONDON"]), + }, +) +# Years around the data year, for frames and inputs that span several. +YEARS = list(range(YEAR - 1, YEAR + 6)) TABLE_SETTINGS = settings( max_examples=200, deadline=None, @@ -1165,6 +1246,7 @@ def test_a_frame_builds_its_people_groups_and_values(population, id_columns): @ROUND_TRIP_SETTINGS @given(population=populations(), id_columns=id_columns) +@example(population=ONE_PERSON, id_columns="both") def test_an_exported_frame_loads_back_and_exports_itself(population, id_columns): simulation = Simulation(dataset=frame(population, id_columns)) exported = simulation.to_input_dataframe() @@ -1187,6 +1269,115 @@ def test_an_exported_frame_loads_back_and_exports_itself(population, id_columns) assert_holds(Simulation(dataset=from_csv), population) +def columns_of(frame_: pd.DataFrame, variable: str) -> list: + return [column for column in frame_ if split_column(column)[0] == variable] + + +def test_an_export_names_the_people_in_each_year_the_rebalancing_records(): + # The rebalancing records the status of a simulation built from data for + # 2025 for 2026 to 2029, where it holds its people and groups by carrying + # them forward from 2025. + exported = Simulation(dataset=frame(ONE_PERSON)).to_input_dataframe() + + status = "uc_receives_new_claimant_health_element" + years = range(YEAR, 2030) + assert columns_of(exported, status) == [f"{status}__{year}" for year in years[1:]] + for identifier in IDENTIFIER_VARIABLES: + columns = columns_of(exported, identifier) + assert columns == [f"{identifier}__{year}" for year in years] + # Together, where the exporter puts an input set for each year. + start = exported.columns.get_loc(columns[0]) + assert exported.columns[start : start + len(columns)].tolist() == columns + for column in columns: + assert exported[column].tolist() == [1] + reloaded = Simulation(dataset=exported) + pd.testing.assert_frame_equal(reloaded.to_input_dataframe(), exported) + + +@pytest.mark.parametrize("year", [YEAR - 1, YEAR + 2, YEAR + 7]) +def test_an_input_set_once_built_for_another_year_round_trips(year): + # Not only the rebalancing: any input set for a year the data does not + # give, before it, among the rebalancing's years or after them. + simulation = Simulation(dataset=frame(ONE_PERSON)) + simulation.set_input("hours_worked", year, np.array([1_000])) + exported = simulation.to_input_dataframe() + + for identifier in IDENTIFIER_VARIABLES: + assert exported[f"{identifier}__{year}"].tolist() == [1] + reloaded = Simulation(dataset=exported) + assert reloaded.calculate("hours_worked", year).tolist() == [1_000] + pd.testing.assert_frame_equal(reloaded.to_input_dataframe(), exported) + + +def multi_year_frame( + population: Population, id_columns: str, identifier_years, data_years +) -> pd.DataFrame: + """The frame of ``population`` with its identifiers given for + ``identifier_years`` and its values for ``data_years``.""" + columns = {} + for year in sorted(set(identifier_years) | set(data_years)): + for column, values in frame(population, id_columns, year).items(): + identifier = split_column(column)[0] in IDENTIFIER_VARIABLES + if year in (identifier_years if identifier else data_years): + columns[column] = values + return pd.DataFrame(columns) + + +@ROUND_TRIP_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), min_size=1, max_size=3), + later_years=st.sets(st.sampled_from(YEARS), max_size=2), +) +@example( + population=ONE_PERSON, + id_columns="both", + identifier_years={YEAR}, + data_years={YEAR}, + later_years=set(), +) +@example( + population=ONE_PERSON, + id_columns="own", + identifier_years={YEAR + 2}, + data_years={YEAR - 1, YEAR + 3}, + later_years={YEAR + 5}, +) +def test_a_frame_over_several_years_exports_the_same_frame_again( + population, id_columns, identifier_years, data_years, later_years +): + # 5. Round trip, for frames that name the identifiers and give values + # for different years, and simulations given an input for other years + # once built. + simulation = Simulation( + dataset=multi_year_frame(population, id_columns, identifier_years, data_years) + ) + for year in sorted(later_years): + simulation.set_input( + "hours_worked", year, population.values["hours_worked"] + year + ) + exported = simulation.to_input_dataframe() + + reloaded = Simulation(dataset=exported) + + pd.testing.assert_frame_equal(reloaded.to_input_dataframe(), exported) + # The export gives the exact identifiers in every year it gives values + # for, and in each year the frame named them for. + years = { + int(period) + for variable, period in map(split_column, exported.columns) + if variable not in IDENTIFIER_VARIABLES and period.isdigit() + } + assert years >= set(data_years) | set(later_years) + for year in years | set(identifier_years): + assert exported[f"person_id__{year}"].tolist() == population.person_ids.tolist() + for group in GROUPS: + exact = population.groups[group].tolist() + assert exported[f"{group}_id__{year}"].tolist() == exact + + @TABLE_SETTINGS @given(population=populations(), id_columns=id_columns, data=st.data()) def test_tables_hold_the_frame_in_any_row_order( From 49e7ef3e01ea4bc20b27c4db19b1930bab9f3d10 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 13:29:48 -0400 Subject: [PATCH 02/12] Give every identifier for each year any column names The loader sets all five identifiers for each whole year an identifier column names, as well as for each year of the dataset, so an export that gives one identifier for a year alone (person_id set by hand for 2032) still re-exported four more columns. The helper now adds every identifier the frame gives for each whole year a model-variable column names, identifier columns included. Also corrects the account of a year before the data, where inputs do not carry back and the exporting simulation's identifiers are zero, and states that the ordering matches the default (sorted) export. Adds an identifier-only case to the regression and the property. Co-Authored-By: Claude Opus 5.5 --- .../fix-roundtrip-recorded-inputs.fixed.md | 2 +- docs/book/usage/simulations.md | 13 ++-- policyengine_uk/data/input_dataframe.py | 52 ++++++++------ policyengine_uk/simulation.py | 6 +- .../tests/test_dataframe_dataset.py | 72 +++++++++++++------ 5 files changed, 90 insertions(+), 55 deletions(-) diff --git a/changelog.d/fix-roundtrip-recorded-inputs.fixed.md b/changelog.d/fix-roundtrip-recorded-inputs.fixed.md index 9d7e60fdcf..69bb07a587 100644 --- a/changelog.d/fix-roundtrip-recorded-inputs.fixed.md +++ b/changelog.d/fix-roundtrip-recorded-inputs.fixed.md @@ -1 +1 @@ -- Fixed `Simulation.to_input_dataframe()` leaving out the person, benefit unit and household identifiers for years in which a simulation was given inputs after it was built, such as the Universal Credit health element status for 2026 to 2029, so that an exported frame now loads back to a simulation that exports the same frame. +- Fixed `Simulation.to_input_dataframe()` leaving out the person, benefit unit and household identifiers for years in which a simulation was given inputs after it was built, such as the Universal Credit health element status for 2026 to 2029, so that an exported frame loads back to a simulation that exports the same frame. diff --git a/docs/book/usage/simulations.md b/docs/book/usage/simulations.md index aa00d945df..7b54a694da 100644 --- a/docs/book/usage/simulations.md +++ b/docs/book/usage/simulations.md @@ -200,12 +200,13 @@ print(sim.calculate("council_tax", 2025)) # One value per household period that is not one year are set on the simulation directly. A variable given for both months and the year is set column by column in the frame's order, since a year's value is spread over the months not already set. -- `to_input_dataframe()` gives the identifier columns for every year it gives - other values for, which is where the dataset the frame loads as holds them. - So an exported frame loads back to a simulation that exports the same frame, - even when the simulation was given inputs for years after its data, such as - the Universal Credit health element status the rebalancing records for 2026 - to 2029. +- `to_input_dataframe()` gives the identifier columns for every year that one + of its columns gives a whole year's value for, since a simulation built from + the frame holds the identifiers in each of those years. So an exported frame + loads back to a simulation that exports the same frame, even when the + simulation was given inputs for years outside its data, such as the + Universal Credit health element status the rebalancing records for 2026 to + 2029. ```{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 60da596084..220aad79dc 100644 --- a/policyengine_uk/data/input_dataframe.py +++ b/policyengine_uk/data/input_dataframe.py @@ -16,10 +16,11 @@ stand for more than one whole number is refused rather than allowed to merge two people or groups. -The identifiers hold for every period, and the dataset a frame loads as holds -them in each year the frame gives data for. ``Simulation.to_input_dataframe`` -writes them for each of those years too (``with_identifiers_in_each_year``), -so that an exported frame loads back to a simulation that exports it again. +The identifiers hold for every period, and a simulation built from a frame +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. """ import warnings @@ -443,28 +444,36 @@ def with_identifiers_in_each_year( df: pd.DataFrame, tax_benefit_system: "TaxBenefitSystem" ) -> pd.DataFrame: """``df``, an exported frame, with each identifier it gives for some year - given for every year it gives other values for. - - The dataset a frame loads as holds the people and groups in each year the - frame gives values for (``dataset_from_input_dataframe``), so a simulation - built from it holds the identifiers as inputs in those years, and exports - them there. A simulation that records an input for a later year once it - is 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, and policyengine-core exports only - the periods an input was set for. Giving the identifiers for that year - makes the frame load back to a simulation that exports the same frame. + given for every whole calendar year a column of the frame names. + + ``dataset_from_input_dataframe`` holds every identifier in each such year: + a year a column of another variable names is a year of the dataset, which + holds the people and groups, and each other year an identifier column + names gets them all as inputs. So a simulation built from the frame holds + the identifiers as inputs in those years, and exports them there. + + The simulation that exported the frame may not have. One given an input + 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. 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 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, where - policyengine-core exports an input set for that year. + 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. - Years are those ``dataset_from_input_dataframe`` makes dataset years: the - whole calendar years that columns of other variables of - ``tax_benefit_system`` name. ``df`` is not changed. + 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. """ named: Dict[str, Dict[int, object]] = {} years = set() @@ -477,8 +486,7 @@ def with_identifiers_in_each_year( continue if variable in IDENTIFIER_VARIABLES: named.setdefault(variable, {})[period.start.year] = column - else: - years.add(period.start.year) + years.add(period.start.year) added = { variable: sorted(years - set(columns)) for variable, columns in named.items() diff --git a/policyengine_uk/simulation.py b/policyengine_uk/simulation.py index a709ffd65d..26bbbe18f7 100644 --- a/policyengine_uk/simulation.py +++ b/policyengine_uk/simulation.py @@ -436,9 +436,9 @@ def to_input_dataframe( ``Simulation(dataset=...)`` loads back. policyengine-core exports each input for the periods it was set for. - The identifiers are also given for every year the frame gives other - values for, as the dataset the frame loads as holds them, so that the - frame loads back to a simulation that exports the same frame. See + 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 ``policyengine_uk.data.input_dataframe.with_identifiers_in_each_year``. Args: diff --git a/policyengine_uk/tests/test_dataframe_dataset.py b/policyengine_uk/tests/test_dataframe_dataset.py index 30d5e9b595..6ba665b8fa 100644 --- a/policyengine_uk/tests/test_dataframe_dataset.py +++ b/policyengine_uk/tests/test_dataframe_dataset.py @@ -33,9 +33,10 @@ This holds for frames that name the identifiers and give values for different years, and for simulations given inputs for other years once built, as the Universal Credit rebalancing gives each benefit unit's - health element status for 2026 to 2029. The export gives the identifiers, - exactly, for every year it gives other values for, which is where the - dataset the frame loads as holds them. + health element status for 2026 to 2029. The export gives every + identifier for each whole year a column names, which is where a + simulation built from it holds them, and person_id and the benefit unit + and household identifiers exactly. 6. Differential: a simulation built from a dataset and one built from the frame it exports give the same value for every variable of the model. 7. Exact names: a column is read only if the part of its name before ``__`` @@ -612,10 +613,11 @@ def test_an_export_gives_its_identifiers_for_each_year_it_gives_values_for( tax_benefit_system, ): # Values for 2024, 2025, 2027 and 2029 make those years of the dataset, - # which holds the identifiers in each. A month, a year only an identifier - # names, and a column that names no variable add none. Each added column - # copies the identifier's latest earlier column, or else its earliest: - # here person_id__2028 is given as floats, so person_id__2029 is too. + # which holds the identifiers in each, and the loader sets them all for + # 2028, which an identifier names. A month and a column that names no + # variable add no year. Each added column copies the identifier's latest + # earlier column, or else its earliest: here person_id__2028 is given as + # floats, so person_id__2029 is too. frame_ = pd.DataFrame( { "benunit_id__2025": [7, 7], @@ -637,6 +639,7 @@ def test_an_export_gives_its_identifiers_for_each_year_it_gives_values_for( "benunit_id__2024", "benunit_id__2025", "benunit_id__2027", + "benunit_id__2028", "benunit_id__2029", "person_id__2024", "person_id__2025", @@ -645,10 +648,15 @@ def test_an_export_gives_its_identifiers_for_each_year_it_gives_values_for( "person_id__2029", *frame_.columns[3:], ] - for year, source in [(2024, 2025), (2027, 2025), (2029, 2028)]: + for identifier, year, source in [ + ("person_id", 2024, 2025), + ("person_id", 2027, 2025), + ("person_id", 2029, 2028), + ("benunit_id", 2028, 2025), + ]: pd.testing.assert_series_equal( - exported[f"person_id__{year}"], - frame_[f"person_id__{source}"], + exported[f"{identifier}__{year}"], + frame_[f"{identifier}__{source}"], check_names=False, ) pd.testing.assert_frame_equal(exported[frame_.columns], frame_) @@ -1294,18 +1302,27 @@ def test_an_export_names_the_people_in_each_year_the_rebalancing_records(): pd.testing.assert_frame_equal(reloaded.to_input_dataframe(), exported) -@pytest.mark.parametrize("year", [YEAR - 1, YEAR + 2, YEAR + 7]) -def test_an_input_set_once_built_for_another_year_round_trips(year): +@pytest.mark.parametrize( + "variable, year, value", + [ + ("hours_worked", YEAR - 1, 1_000), + ("hours_worked", YEAR + 2, 1_000), + ("hours_worked", YEAR + 7, 1_000), + # One identifier for a year: the loader sets them all for it. + ("person_id", YEAR + 7, 1), + ], +) +def test_an_input_set_once_built_for_another_year_round_trips(variable, year, value): # Not only the rebalancing: any input set for a year the data does not # give, before it, among the rebalancing's years or after them. simulation = Simulation(dataset=frame(ONE_PERSON)) - simulation.set_input("hours_worked", year, np.array([1_000])) + simulation.set_input(variable, year, np.array([value])) exported = simulation.to_input_dataframe() for identifier in IDENTIFIER_VARIABLES: assert exported[f"{identifier}__{year}"].tolist() == [1] reloaded = Simulation(dataset=exported) - assert reloaded.calculate("hours_worked", year).tolist() == [1_000] + assert reloaded.calculate(variable, year).tolist() == [value] pd.testing.assert_frame_equal(reloaded.to_input_dataframe(), exported) @@ -1330,6 +1347,7 @@ def multi_year_frame( identifier_years=st.sets(st.sampled_from(YEARS), min_size=1, max_size=2), data_years=st.sets(st.sampled_from(YEARS), min_size=1, max_size=3), later_years=st.sets(st.sampled_from(YEARS), max_size=2), + later_identifier_years=st.sets(st.sampled_from(YEARS), max_size=1), ) @example( population=ONE_PERSON, @@ -1337,6 +1355,7 @@ def multi_year_frame( identifier_years={YEAR}, data_years={YEAR}, later_years=set(), + later_identifier_years=set(), ) @example( population=ONE_PERSON, @@ -1344,13 +1363,19 @@ def multi_year_frame( identifier_years={YEAR + 2}, data_years={YEAR - 1, YEAR + 3}, later_years={YEAR + 5}, + later_identifier_years={YEAR + 4}, ) def test_a_frame_over_several_years_exports_the_same_frame_again( - population, id_columns, identifier_years, data_years, later_years + population, + id_columns, + identifier_years, + data_years, + later_years, + later_identifier_years, ): # 5. Round trip, for frames that name the identifiers and give values - # for different years, and simulations given an input for other years - # once built. + # for different years, and simulations given an input, or a person's + # identifier, for other years once built. simulation = Simulation( dataset=multi_year_frame(population, id_columns, identifier_years, data_years) ) @@ -1358,20 +1383,21 @@ def test_a_frame_over_several_years_exports_the_same_frame_again( simulation.set_input( "hours_worked", year, population.values["hours_worked"] + year ) + for year in sorted(later_identifier_years): + simulation.set_input("person_id", year, population.person_ids) exported = simulation.to_input_dataframe() reloaded = Simulation(dataset=exported) pd.testing.assert_frame_equal(reloaded.to_input_dataframe(), exported) - # The export gives the exact identifiers in every year it gives values - # for, and in each year the frame named them for. + # The export gives the exact identifiers in every whole year it names. years = { int(period) - for variable, period in map(split_column, exported.columns) - if variable not in IDENTIFIER_VARIABLES and period.isdigit() + for _, period in map(split_column, exported.columns) + if period.isdigit() } - assert years >= set(data_years) | set(later_years) - for year in years | set(identifier_years): + assert years >= set(data_years) | set(later_years) | set(identifier_years) + for year in years: assert exported[f"person_id__{year}"].tolist() == population.person_ids.tolist() for group in GROUPS: exact = population.groups[group].tolist() From a9664455e12e415ab96dd5cce7f0fe03f14a75a1 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 14:02:52 -0400 Subject: [PATCH 03/12] Reflow the with_identifiers_in_each_year docstring Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/data/input_dataframe.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/policyengine_uk/data/input_dataframe.py b/policyengine_uk/data/input_dataframe.py index 220aad79dc..6ff5c077a4 100644 --- a/policyengine_uk/data/input_dataframe.py +++ b/policyengine_uk/data/input_dataframe.py @@ -458,10 +458,9 @@ def with_identifiers_in_each_year( 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. + 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 From 426df7b49efa1a6f2bd7428b1009224255ed93f2 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 16:36:54 -0400 Subject: [PATCH 04/12] 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 05/12] 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 06/12] 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 07/12] 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 08/12] 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 09/12] 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 10/12] 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 11/12] 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 12/12] 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)