From 5afccfde39092f9fba1b6d158048d4f843559016 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 10 Oct 2026 17:07:33 -0400 Subject: [PATCH 1/4] Store person_household_id and person_benunit_id as integers Both membership variables were declared float, so policyengine-core stored them as float32, which holds whole numbers exactly only up to 2**24. Identifiers near 900 million read back rounded: calculate, to_input_dataframe() and UKSingleYearDataset.from_simulation returned 900000000.0 for three different households, and a simulation built from a policyengine-core dataset file (rebuilt from that export) merged them into one household. Declare both int, like person_id, benunit_id and household_id, so they are stored as int32 and are exact up to 2**31 - 1. Every cached UK dataset's identifiers fit (the largest seen is 967,010,009). build_from_ids now refuses identifiers outside the int32 range or not whole, which the store's astype would otherwise wrap or truncate without an error. Fixes #2267 Co-Authored-By: Claude Opus 5.5 --- .../fix-person-entity-id-int-2267.fixed.md | 1 + docs/book/usage/simulations.md | 6 + policyengine_uk/simulation.py | 19 + policyengine_uk/tests/test_entity_ids.py | 509 ++++++++++++++++++ policyengine_uk/utils/entity_ids.py | 64 +++ .../demographic/person_benunit_id.py | 2 +- .../demographic/person_household_id.py | 2 +- 7 files changed, 601 insertions(+), 2 deletions(-) create mode 100644 changelog.d/fix-person-entity-id-int-2267.fixed.md create mode 100644 policyengine_uk/tests/test_entity_ids.py create mode 100644 policyengine_uk/utils/entity_ids.py diff --git a/changelog.d/fix-person-entity-id-int-2267.fixed.md b/changelog.d/fix-person-entity-id-int-2267.fixed.md new file mode 100644 index 0000000000..7e5f7bdc70 --- /dev/null +++ b/changelog.d/fix-person-entity-id-int-2267.fixed.md @@ -0,0 +1 @@ +- Store `person_household_id` and `person_benunit_id` as integers, like the other identifier variables, so identifiers above 16,777,216 read back exactly from `calculate`, `to_input_dataframe()` and `UKSingleYearDataset.from_simulation`. A simulation built from a policyengine-core dataset file no longer merges households or benefit units whose identifiers rounded to the same number. A simulation now refuses a dataset whose identifiers are not whole numbers from -2,147,483,648 to 2,147,483,647, instead of storing them as different numbers. diff --git a/docs/book/usage/simulations.md b/docs/book/usage/simulations.md index 2ff8dc3f9c..d6266e84e8 100644 --- a/docs/book/usage/simulations.md +++ b/docs/book/usage/simulations.md @@ -238,6 +238,12 @@ Each table should contain the ID columns needed to link entities together: - `benunit`: `benunit_id`, plus any benefit-unit variables - `household`: `household_id`, plus any household-level variables +Whatever the data source, identifiers must be whole numbers from -2,147,483,648 +to 2,147,483,647. A simulation stores them as 32-bit integers and refuses a +dataset with an identifier outside that range, so the identifiers it returns +(for example `sim.calculate("person_household_id", 2025)`) are the ones in your +tables. + Use `UKSingleYearDataset` when you have one cross-section and want PolicyEngine UK to extend it forward using its standard uprating assumptions. diff --git a/policyengine_uk/simulation.py b/policyengine_uk/simulation.py index bf21684c48..98666e6e7a 100644 --- a/policyengine_uk/simulation.py +++ b/policyengine_uk/simulation.py @@ -26,6 +26,7 @@ UKMultiYearDataset, UKSingleYearDataset, ) +from policyengine_uk.utils.entity_ids import ENTITY_ID_VARIABLES, check_entity_ids from policyengine_uk.utils.scenario import Scenario from policyengine_uk.data.economic_assumptions import ( extend_single_year_dataset, @@ -553,11 +554,29 @@ def build_from_ids( person_household_id: Array mapping persons to households benunit_id: Array of benefit unit IDs household_id: Array of household IDs + + Raises: + ValueError: If an ID is not a whole number, or is outside the + range of the variable that stores it. """ from policyengine_core.simulations.simulation_builder import ( SimulationBuilder, ) # Import here to avoid circular dependency + # The callers store these columns as the variables' values after + # building the entities, and exports and rebuilds read them from there. + for variable_name, ids in zip( + ENTITY_ID_VARIABLES, + ( + person_id, + person_benunit_id, + person_household_id, + benunit_id, + household_id, + ), + ): + check_entity_ids(self.tax_benefit_system.variables[variable_name], ids) + builder = SimulationBuilder() builder.populations = self.tax_benefit_system.instantiate_entities() diff --git a/policyengine_uk/tests/test_entity_ids.py b/policyengine_uk/tests/test_entity_ids.py new file mode 100644 index 0000000000..4838f2e2b2 --- /dev/null +++ b/policyengine_uk/tests/test_entity_ids.py @@ -0,0 +1,509 @@ +"""Entity identifiers read back from a simulation exactly as they were given. + +A simulation built from data stores each identifier column as the values of a +variable: ``person_id``, ``person_benunit_id``, ``person_household_id``, +``benunit_id`` and ``household_id``. ``calculate``, ``to_input_dataframe`` and +``UKSingleYearDataset.from_simulation`` return those stored values, and +``Simulation.build_from_dataset`` rebuilds the entities from the last of them. +``person_benunit_id`` and ``person_household_id`` used to be stored as 32-bit +floats, which hold whole numbers exactly only up to 2**24, so larger +identifiers read back rounded and a rebuild merged the households that shared +a rounded value (PolicyEngine/policyengine-uk#2267). + +Invariants, for identifiers that are whole numbers from -2**31 to 2**31 - 1 +(the range of the 32-bit integers that store them): + +1. ``calculate``, ``to_input_dataframe`` and + ``UKSingleYearDataset.from_simulation`` return every identifier column + exactly, as integers, whether the dataset holds it as integers or floats. +2. The stored memberships equal the populations' memberships: a person's + ``person_household_id`` is the identifier of the household the populations + place the person in, and likewise for benefit units. +3. The entity structure is the one the dataset's tables describe, whatever + the size of the identifiers. +4. A simulation built from a policyengine-core dataset file, which is rebuilt + from its own export, has that same structure and those same identifiers. +5. ``check_entity_ids`` accepts exactly the arrays that the storing variable's + dtype holds unchanged. So an identifier outside the range, or not a whole + number, raises ``ValueError`` rather than being stored as another number. + +The group tables are sorted by identifier, as ``build_from_ids`` numbers the +benefit units and households in that order, and every group has a member. +""" + +import warnings + +import h5py +import numpy as np +import pandas as pd +import pytest +from hypothesis import example, given, settings +from hypothesis import strategies as st + +from policyengine_uk import Microsimulation +from policyengine_uk.data import UKMultiYearDataset, UKSingleYearDataset +from policyengine_uk.system import system +from policyengine_uk.utils.entity_ids import ENTITY_ID_VARIABLES, check_entity_ids + +pytestmark = pytest.mark.usefixtures("cloned_uk_tax_benefit_system") + +YEAR = 2025 +ID_MIN, ID_MAX = -(2**31), 2**31 - 1 +# The first whole number a 32-bit float cannot hold is 2**24 + 1. +FLOAT32_EXACT_LIMIT = 2**24 +# Three households that a 32-bit float stores as the same number, 900,000,000. +ISSUE_IDS = [900_000_001, 900_000_002, 900_000_003] + +# The person table's column that holds each identifier once per person. +PERSON_LEVEL_COLUMN = { + "person_id": "person_id", + "person_benunit_id": "person_benunit_id", + "person_household_id": "person_household_id", + "benunit_id": "person_benunit_id", + "household_id": "person_household_id", +} + +identifiers = st.one_of( + st.integers(ID_MIN, ID_MAX), + st.integers(ID_MAX - 40, ID_MAX), + st.integers(ID_MIN, ID_MIN + 40), + st.integers(FLOAT32_EXACT_LIMIT - 20, FLOAT32_EXACT_LIMIT + 40), + st.integers(900_000_000, 900_000_040), + st.integers(0, 40), +) + + +def tables(person_ids, person_benunit_ids, person_household_ids, id_dtype="int64"): + """Person, benefit unit and household tables for the given memberships. + + The household table holds the columns that extending a dataset to later + years reads. + """ + person = pd.DataFrame( + { + "person_id": person_ids, + "person_benunit_id": person_benunit_ids, + "person_household_id": person_household_ids, + } + ).astype(id_dtype) + person["age"] = 40 + benunit = pd.DataFrame({"benunit_id": sorted(set(person_benunit_ids))}) + household = pd.DataFrame({"household_id": sorted(set(person_household_ids))}) + household = household.astype(id_dtype).assign( + household_weight=1.0, + region="LONDON", + rent=0.0, + tenure_type="OWNED_OUTRIGHT", + council_tax=0.0, + ) + return person, benunit.astype(id_dtype), household + + +@st.composite +def memberships(draw): + """Up to four households of one to three benefit units of one to three + people, with distinct identifiers of any size and people in any order.""" + sizes = draw( + st.lists( + st.lists(st.integers(1, 3), min_size=1, max_size=3), + min_size=1, + max_size=4, + ) + ) + benunit_household = [h for h, units in enumerate(sizes) for _ in units] + benunit_sizes = [size for units in sizes for size in units] + person_benunit = [b for b, size in enumerate(benunit_sizes) for _ in range(size)] + + def distinct(count): + return draw(st.lists(identifiers, min_size=count, max_size=count, unique=True)) + + household_ids = distinct(len(sizes)) + benunit_ids = distinct(len(benunit_sizes)) + person_ids = distinct(len(person_benunit)) + order = draw(st.permutations(range(len(person_benunit)))) + return ( + [person_ids[i] for i in order], + [benunit_ids[person_benunit[i]] for i in order], + [household_ids[benunit_household[person_benunit[i]]] for i in order], + ) + + +def simulate(person, benunit, household, years=(YEAR,)): + """A simulation of the tables as given, with no uprating to other years.""" + return Microsimulation( + dataset=UKMultiYearDataset( + datasets=[ + UKSingleYearDataset( + person=person.copy(), + benunit=benunit.copy(), + household=household.copy(), + fiscal_year=year, + ) + for year in years + ] + ) + ) + + +def write_core_dataset_file(path, person, benunit, household): + """Save the tables as policyengine-core stores a dataset: one array per + variable and period, with text as bytes.""" + with h5py.File(path, "w") as file: + for table in (person, benunit, household): + for column in table.columns: + values = table[column].to_numpy() + if values.dtype.kind == "O": + values = values.astype("S") + file.create_dataset(f"{column}/{YEAR}", data=values) + return str(path) + + +def whole(values) -> list[int]: + return [int(value) for value in values] + + +def expected_identifiers(person, benunit, household) -> dict[str, list[int]]: + return { + "person_id": whole(person.person_id), + "person_benunit_id": whole(person.person_benunit_id), + "person_household_id": whole(person.person_household_id), + "benunit_id": whole(benunit.benunit_id), + "household_id": whole(household.household_id), + } + + +def stored(sim, variable: str, year: int = YEAR) -> list[int]: + """The variable's stored values, which must be integers.""" + values = np.asarray(sim.calculate(variable, year)) + assert values.dtype.kind == "i", (variable, values.dtype) + return values.tolist() + + +def rows_in_group_table(group_ids, person_group_ids) -> list[int]: + """The row of each person's group in the group table.""" + row = {int(identifier): index for index, identifier in enumerate(group_ids)} + return [row[int(identifier)] for identifier in person_group_ids] + + +def assert_structure_matches_tables(sim, person, benunit, household): + """Invariant 3: the populations hold the tables' memberships.""" + assert sim.populations["person"].ids.tolist() == whole(person.person_id) + for entity, group_ids, person_group_ids in ( + ("benunit", benunit.benunit_id, person.person_benunit_id), + ("household", household.household_id, person.person_household_id), + ): + population = sim.populations[entity] + assert population.ids.tolist() == whole(group_ids) + assert population.members_entity_id.tolist() == rows_in_group_table( + group_ids, person_group_ids + ) + + +def assert_stored_memberships_match_populations(sim, year: int = YEAR): + """Invariant 2: the stored memberships are the populations' memberships.""" + for entity in ("benunit", "household"): + population = sim.populations[entity] + from_populations = population.ids[population.members_entity_id].tolist() + assert stored(sim, f"person_{entity}_id", year) == from_populations + assert stored(sim, f"{entity}_id", year) == population.ids.tolist() + + +def assert_identifiers_read_back(sim, person, benunit, household, years=(YEAR,)): + """Invariant 1, for each way of reading the stored identifiers.""" + expected = expected_identifiers(person, benunit, household) + frame = sim.to_input_dataframe() + for year in years: + for variable in ENTITY_ID_VARIABLES: + assert stored(sim, variable, year) == expected[variable], variable + + # One row per person, so a group's identifier repeats for each member. + for variable in ENTITY_ID_VARIABLES: + column = frame[f"{variable}__{year}"] + assert column.dtype.kind == "i", (variable, column.dtype) + assert column.tolist() == expected[PERSON_LEVEL_COLUMN[variable]], variable + + export = UKSingleYearDataset.from_simulation(sim, fiscal_year=year) + for table, variables in ( + (export.person, ENTITY_ID_VARIABLES[:3]), + (export.benunit, ("benunit_id",)), + (export.household, ("household_id",)), + ): + for variable in variables: + assert table[variable].dtype.kind == "i", (variable, table.dtypes) + assert table[variable].tolist() == expected[variable], variable + + +@pytest.mark.parametrize("variable", ENTITY_ID_VARIABLES) +def test_identifier_variables_are_stored_as_integers(variable): + """All five identifier variables share one integer store, so a membership + column can hold any identifier its group's own column can.""" + definition = system.variables[variable] + assert definition.value_type is int + assert definition.dtype == system.variables["household_id"].dtype + assert np.iinfo(definition.dtype).min <= ID_MIN + assert np.iinfo(definition.dtype).max >= ID_MAX + + +@given(members=memberships(), id_dtype=st.sampled_from(["int64", "float64"])) +@example(members=(ISSUE_IDS, ISSUE_IDS, ISSUE_IDS), id_dtype="int64") +@example( + members=( + [ID_MAX, ID_MAX - 1, ID_MIN, ID_MIN + 1], + [ID_MAX, ID_MAX, ID_MIN, ID_MAX - 1], + [ID_MAX, ID_MAX, ID_MIN, ID_MAX], + ), + id_dtype="float64", +) +@example( + members=( + [1, 2, 3], + [FLOAT32_EXACT_LIMIT + 1, FLOAT32_EXACT_LIMIT, FLOAT32_EXACT_LIMIT + 1], + [FLOAT32_EXACT_LIMIT + 1, FLOAT32_EXACT_LIMIT, FLOAT32_EXACT_LIMIT + 1], + ), + id_dtype="int64", +) +@settings(max_examples=30, deadline=None) +def test_identifiers_in_range_read_back_exactly(members, id_dtype): + """Invariants 1 to 3 for a simulation built from entity tables.""" + person, benunit, household = tables(*members, id_dtype=id_dtype) + sim = simulate(person, benunit, household) + assert_structure_matches_tables(sim, person, benunit, household) + assert_stored_memberships_match_populations(sim) + assert_identifiers_read_back(sim, person, benunit, household) + + +@given(members=memberships(), id_dtype=st.sampled_from(["int64", "float64"])) +@example(members=(ISSUE_IDS, ISSUE_IDS, ISSUE_IDS), id_dtype="float64") +@settings(max_examples=10, deadline=None) +def test_core_dataset_file_builds_the_tables_structure( + members, id_dtype, tmp_path_factory +): + """Invariant 4. ``build_from_dataset`` builds the entities, exports them + with ``UKSingleYearDataset.from_simulation`` and rebuilds from the export, + so a rounded membership column changed who lives with whom.""" + person, benunit, household = tables(*members, id_dtype=id_dtype) + path = write_core_dataset_file( + tmp_path_factory.mktemp("core_dataset") / "dataset.h5", + person, + benunit, + household, + ) + sim = Microsimulation(dataset=path) + assert_structure_matches_tables(sim, person, benunit, household) + assert_stored_memberships_match_populations(sim) + expected = expected_identifiers(person, benunit, household) + for variable in ENTITY_ID_VARIABLES: + assert stored(sim, variable) == expected[variable], variable + + +@pytest.fixture(scope="module") +def issue_tables(): + return tables(ISSUE_IDS, ISSUE_IDS, ISSUE_IDS) + + +@pytest.fixture(scope="module") +def issue_simulation(cloned_uk_tax_benefit_system, issue_tables): + """The reproduction in the issue: a single-year dataset, which the + simulation extends to later years.""" + person, benunit, household = issue_tables + return Microsimulation( + dataset=UKSingleYearDataset( + person=person.copy(), + benunit=benunit.copy(), + household=household.copy(), + fiscal_year=YEAR, + ) + ) + + +def test_issue_reproduction_reads_back_in_every_year(issue_simulation, issue_tables): + years = issue_simulation.dataset.years + assert len(years) > 1 + assert_identifiers_read_back(issue_simulation, *issue_tables, years=years) + for year in years: + assert_stored_memberships_match_populations(issue_simulation, year) + + +def test_issue_reproduction_keeps_three_households(issue_simulation): + assert stored(issue_simulation, "person_household_id") == ISSUE_IDS + assert stored(issue_simulation, "person_benunit_id") == ISSUE_IDS + assert stored(issue_simulation, "household_count_people") == [1, 1, 1] + + +def test_core_dataset_file_keeps_households_apart(tmp_path): + """A policyengine-core dataset file with float identifiers, as an older + UK dataset has. Before the fix its three one-person households loaded as + one household of three and two empty households.""" + person, benunit, household = tables( + ISSUE_IDS, ISSUE_IDS, ISSUE_IDS, id_dtype="float64" + ) + path = write_core_dataset_file(tmp_path / "dataset.h5", person, benunit, household) + sim = Microsimulation(dataset=path) + assert sim.populations["household"].members_entity_id.tolist() == [0, 1, 2] + assert sim.populations["benunit"].members_entity_id.tolist() == [0, 1, 2] + assert stored(sim, "household_count_people") == [1, 1, 1] + assert stored(sim, "person_household_id") == ISSUE_IDS + + +def test_exported_frame_rebuilds_the_same_households(issue_simulation): + """A frame from ``to_input_dataframe`` rebuilds the households it came + from. Each household here has one person: a frame with a larger household + does not load yet (PolicyEngine/policyengine-uk#2266).""" + rebuilt = Microsimulation(dataset=issue_simulation.to_input_dataframe()) + assert rebuilt.populations["household"].ids.tolist() == ISSUE_IDS + assert rebuilt.populations["household"].members_entity_id.tolist() == [0, 1, 2] + assert rebuilt.populations["benunit"].members_entity_id.tolist() == [0, 1, 2] + assert stored(rebuilt, "person_household_id") == ISSUE_IDS + assert stored(rebuilt, "household_count_people") == [1, 1, 1] + + +def test_float32_identifier_columns_load_when_they_are_whole(): + """A 32-bit float column is exact below 2**24, and loads as integers.""" + ids = [FLOAT32_EXACT_LIMIT - 2, FLOAT32_EXACT_LIMIT - 1, FLOAT32_EXACT_LIMIT] + person, benunit, household = tables(ids, ids, ids, id_dtype="float32") + sim = simulate(person, benunit, household) + assert_identifiers_read_back(sim, person, benunit, household) + + +def storable_values(variable: str): + """Arrays of the kinds an identifier column can have, in and out of the + storing variable's range, including floats that are not whole numbers.""" + limits = np.iinfo(system.variables[variable].dtype) + near_limits = st.one_of( + st.integers(limits.min - 3, limits.min + 3), + st.integers(limits.max - 3, limits.max + 3), + st.integers(-(2**24) - 3, 2**24 + 3), + ) + + def integers(dtype): + bounds = np.iinfo(dtype) + return st.lists( + st.one_of( + st.integers(bounds.min, bounds.max), + near_limits.map(lambda value: min(max(value, bounds.min), bounds.max)), + ), + max_size=6, + ).map(lambda values: np.array(values, dtype=dtype)) + + def floats(dtype): + return st.lists( + st.one_of( + near_limits.map(float), + st.floats(allow_nan=True, allow_infinity=True, width=32), + st.floats(min_value=-(2.0**33), max_value=2.0**33), + ), + max_size=6, + ).map(lambda values: np.array(values, dtype=dtype)) + + return st.one_of( + integers(np.int64), + integers(np.int32), + integers(np.uint32), + integers(np.uint64), + floats(np.float64), + floats(np.float32), + ) + + +def held_unchanged(values: np.ndarray, dtype) -> bool: + """Whether policyengine-core's cast to the variable's dtype + (``Holder._to_array``) keeps every value.""" + with warnings.catch_warnings(): + # NumPy warns when a float is outside the integer range. + warnings.simplefilter("ignore", RuntimeWarning) + cast = values.astype(dtype) + # Python compares integers with floats exactly; NaN equals nothing. + return cast.tolist() == values.tolist() + + +@pytest.mark.parametrize("variable", ENTITY_ID_VARIABLES) +@given(data=st.data()) +@settings(max_examples=300, deadline=None) +def test_check_accepts_exactly_what_the_store_holds_unchanged(variable, data): + """Invariant 5, against the cast that stores the values.""" + definition = system.variables[variable] + values = data.draw(storable_values(variable)) + try: + check_entity_ids(definition, values) + accepted = True + except ValueError as error: + assert variable in str(error) + accepted = False + assert accepted == held_unchanged(values, definition.dtype), values + + +def test_check_leaves_other_kinds_of_identifier_to_the_store(): + """Text and other non-numeric identifiers are not checked here; + policyengine-core reports those it cannot store.""" + definition = system.variables["person_id"] + check_entity_ids(definition, np.array(["a", "b"])) + check_entity_ids(definition, np.array([True, False])) + check_entity_ids(definition, np.array([], dtype=np.float64)) + + +OUT_OF_RANGE_IDS = [ID_MAX - 1, ID_MAX, ID_MAX + 1] + + +@pytest.mark.parametrize( + "entity, variable", + [ + ("person", "person_id"), + ("benunit", "person_benunit_id"), + ("household", "person_household_id"), + ], +) +@pytest.mark.parametrize("id_dtype", ["int64", "float64"]) +def test_identifier_above_the_range_is_refused(entity, variable, id_dtype): + """An identifier one above the range is refused when the entities are + built, whichever entity it names.""" + ids = {name: [1, 2, 3] for name in ("person", "benunit", "household")} + ids[entity] = OUT_OF_RANGE_IDS + person, benunit, household = tables( + ids["person"], ids["benunit"], ids["household"], id_dtype=id_dtype + ) + with pytest.raises(ValueError, match=f"{variable} ranges from .* to {ID_MAX + 1}"): + simulate(person, benunit, household) + + +def test_identifier_below_the_range_is_refused(): + ids = [ID_MIN - 1, ID_MIN, ID_MIN + 1] + with pytest.raises(ValueError, match=f"person_id ranges from {ID_MIN - 1} to"): + simulate(*tables(ids, ids, ids)) + + +def test_identifier_that_is_not_a_whole_number_is_refused(): + person, benunit, household = tables([1, 2, 3], [1, 2, 3], [1, 2, 3], "float64") + person["person_household_id"] = [1.0, 2.5, 3.0] + with pytest.raises(ValueError, match="person_household_id must hold whole"): + simulate(person, benunit, household) + + +def test_single_year_dataset_refuses_an_identifier_out_of_range(): + person, benunit, household = tables( + OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS + ) + dataset = UKSingleYearDataset( + person=person, benunit=benunit, household=household, fiscal_year=YEAR + ) + with pytest.raises(ValueError, match="person_id ranges from"): + Microsimulation(dataset=dataset) + + +def test_core_dataset_file_refuses_an_identifier_out_of_range(tmp_path): + person, benunit, household = tables( + OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS + ) + path = write_core_dataset_file(tmp_path / "dataset.h5", person, benunit, household) + with pytest.raises(ValueError, match="person_id ranges from"): + Microsimulation(dataset=path) + + +def test_frame_refuses_an_identifier_out_of_range(): + frame = pd.DataFrame( + { + f"{variable}__{YEAR}": OUT_OF_RANGE_IDS + for variable in (*ENTITY_ID_VARIABLES, "age") + } + ) + with pytest.raises(ValueError, match="person_id ranges from"): + Microsimulation(dataset=frame) diff --git a/policyengine_uk/utils/entity_ids.py b/policyengine_uk/utils/entity_ids.py new file mode 100644 index 0000000000..c548e8b2b6 --- /dev/null +++ b/policyengine_uk/utils/entity_ids.py @@ -0,0 +1,64 @@ +"""Entity identifiers a simulation can store exactly. + +A simulation built from data keeps each identifier column in two places. The +populations hold the entity structure (``ids`` and ``members_entity_id``). +The variables in ``ENTITY_ID_VARIABLES`` hold the same identifiers as stored +values, and those are what ``calculate``, ``to_input_dataframe`` and +``UKSingleYearDataset.from_simulation`` return. ``Simulation.build_from_dataset`` +rebuilds the entities from that last export. + +policyengine-core casts a stored value to its variable's dtype with ``astype`` +(``Holder._to_array``). That cast wraps an integer outside the dtype's range and +drops the fraction of a float, both without an error. ``check_entity_ids`` +refuses such identifiers when the entities are built, so that every identifier +a simulation accepts reads back exactly. +""" + +import numpy as np +from policyengine_core.variables import Variable + +# The variables that store the identifier columns ``build_from_ids`` takes, in +# the order it takes them. +ENTITY_ID_VARIABLES = ( + "person_id", + "person_benunit_id", + "person_household_id", + "benunit_id", + "household_id", +) + + +def check_entity_ids(variable: Variable, values) -> None: + """Raise ``ValueError`` if ``variable`` cannot store ``values`` exactly. + + Only integer and float arrays are checked against an integer variable. + policyengine-core reports any other mismatch when it stores the values. + + Args: + variable: The variable that will store the identifiers. + values: The identifiers, one per record. + """ + values = np.asarray(values) + if values.size == 0 or values.dtype.kind not in "iuf": + return + if not np.issubdtype(variable.dtype, np.integer): + return + if values.dtype.kind == "f": + whole = np.isfinite(values) & (values == np.trunc(values)) + if not whole.all(): + raise ValueError( + f"{variable.name} must hold whole numbers, but " + f"{int((~whole).sum())} of its {values.size} values are not " + f"(the first is {values[~whole][0]})." + ) + # Python integers compare exactly. A float32 or unsigned 64-bit bound does + # not compare exactly with a NumPy integer limit. + lowest, highest = int(values.min()), int(values.max()) + limits = np.iinfo(variable.dtype) + if lowest < limits.min or highest > limits.max: + raise ValueError( + f"{variable.name} ranges from {lowest} to {highest}, but a " + f"simulation stores it as {np.dtype(variable.dtype).name}, which " + f"holds whole numbers from {limits.min} to {limits.max}. Renumber " + "the identifiers to fit that range." + ) diff --git a/policyengine_uk/variables/household/demographic/person_benunit_id.py b/policyengine_uk/variables/household/demographic/person_benunit_id.py index 2714bdb019..90c5913886 100644 --- a/policyengine_uk/variables/household/demographic/person_benunit_id.py +++ b/policyengine_uk/variables/household/demographic/person_benunit_id.py @@ -2,7 +2,7 @@ class person_benunit_id(Variable): - value_type = float + value_type = int label = "Person's benefit unit ID" entity = Person definition_period = YEAR diff --git a/policyengine_uk/variables/household/demographic/person_household_id.py b/policyengine_uk/variables/household/demographic/person_household_id.py index b82a0a58bd..f2784da5e5 100644 --- a/policyengine_uk/variables/household/demographic/person_household_id.py +++ b/policyengine_uk/variables/household/demographic/person_household_id.py @@ -2,7 +2,7 @@ class person_household_id(Variable): - value_type = float + value_type = int label = "Person's household ID" entity = Person definition_period = YEAR From 608a859e92c8494fb31d1ba56b19a124a7169993 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 10 Oct 2026 17:30:58 -0400 Subject: [PATCH 2/4] Warn about identifiers out of range instead of refusing them Data builds can number identifiers past the int32 range. uk-data's clone_and_assign offsets clone k by k * 10 ** (digits of the largest id), so a local build of the 2024-25 FRS at its default of 10 clones would reach about 9.1 billion (its release and PR builds use one clone). Microcosm's UK build sizes its clone offsets the same way (id_multiplier_for_values) on ids that already reach 967 million, and its policyengine-uk adapter passes frames to UKSingleYearDataset without renumbering them. person_id, benunit_id and household_id already wrap out-of-range ids without an error, while the populations, built from the given arrays, stay exact. Refusing such ids could turn those builds from wrong stored ids into failures, so build_from_ids gives an EntityIdWarning instead. In-range ids are unchanged: they read back exactly and raise no warning. Co-Authored-By: Claude Opus 5.5 --- .../fix-person-entity-id-int-2267.fixed.md | 2 +- docs/book/usage/simulations.md | 11 +- policyengine_uk/simulation.py | 8 +- policyengine_uk/tests/test_entity_ids.py | 133 +++++++++++------- policyengine_uk/utils/entity_ids.py | 62 +++++--- 5 files changed, 139 insertions(+), 77 deletions(-) diff --git a/changelog.d/fix-person-entity-id-int-2267.fixed.md b/changelog.d/fix-person-entity-id-int-2267.fixed.md index 7e5f7bdc70..7ecb743199 100644 --- a/changelog.d/fix-person-entity-id-int-2267.fixed.md +++ b/changelog.d/fix-person-entity-id-int-2267.fixed.md @@ -1 +1 @@ -- Store `person_household_id` and `person_benunit_id` as integers, like the other identifier variables, so identifiers above 16,777,216 read back exactly from `calculate`, `to_input_dataframe()` and `UKSingleYearDataset.from_simulation`. A simulation built from a policyengine-core dataset file no longer merges households or benefit units whose identifiers rounded to the same number. A simulation now refuses a dataset whose identifiers are not whole numbers from -2,147,483,648 to 2,147,483,647, instead of storing them as different numbers. +- Store `person_household_id` and `person_benunit_id` as integers, like the other identifier variables, so identifiers above 16,777,216 read back exactly from `calculate`, `to_input_dataframe()` and `UKSingleYearDataset.from_simulation`. A simulation built from a policyengine-core dataset file no longer merges households or benefit units whose identifiers rounded to the same number. A simulation now gives an `EntityIdWarning` when an identifier is not a whole number from -2,147,483,648 to 2,147,483,647, the range it can store exactly. diff --git a/docs/book/usage/simulations.md b/docs/book/usage/simulations.md index d6266e84e8..b11d670d50 100644 --- a/docs/book/usage/simulations.md +++ b/docs/book/usage/simulations.md @@ -238,11 +238,12 @@ Each table should contain the ID columns needed to link entities together: - `benunit`: `benunit_id`, plus any benefit-unit variables - `household`: `household_id`, plus any household-level variables -Whatever the data source, identifiers must be whole numbers from -2,147,483,648 -to 2,147,483,647. A simulation stores them as 32-bit integers and refuses a -dataset with an identifier outside that range, so the identifiers it returns -(for example `sim.calculate("person_household_id", 2025)`) are the ones in your -tables. +Whatever the data source, keep identifiers to whole numbers from +-2,147,483,648 to 2,147,483,647. A simulation stores them as 32-bit integers, +so within that range the identifiers it returns (for example +`sim.calculate("person_household_id", 2025)`) are the ones in your tables. +Outside it, the simulation gives an `EntityIdWarning`: the identifiers it +returns differ from yours, and so may anything rebuilt from them. Use `UKSingleYearDataset` when you have one cross-section and want PolicyEngine UK to extend it forward using its standard uprating assumptions. diff --git a/policyengine_uk/simulation.py b/policyengine_uk/simulation.py index 98666e6e7a..30c4925090 100644 --- a/policyengine_uk/simulation.py +++ b/policyengine_uk/simulation.py @@ -555,9 +555,10 @@ def build_from_ids( benunit_id: Array of benefit unit IDs household_id: Array of household IDs - Raises: - ValueError: If an ID is not a whole number, or is outside the - range of the variable that stores it. + Warns: + EntityIdWarning: If an ID is not a whole number, or is outside the + range of the variable that stores it, so that the stored ID + differs from the one given. """ from policyengine_core.simulations.simulation_builder import ( SimulationBuilder, @@ -565,6 +566,7 @@ def build_from_ids( # The callers store these columns as the variables' values after # building the entities, and exports and rebuilds read them from there. + # The populations below take the arrays as given. for variable_name, ids in zip( ENTITY_ID_VARIABLES, ( diff --git a/policyengine_uk/tests/test_entity_ids.py b/policyengine_uk/tests/test_entity_ids.py index 4838f2e2b2..d6fcb59e63 100644 --- a/policyengine_uk/tests/test_entity_ids.py +++ b/policyengine_uk/tests/test_entity_ids.py @@ -23,9 +23,13 @@ the size of the identifiers. 4. A simulation built from a policyengine-core dataset file, which is rebuilt from its own export, has that same structure and those same identifiers. -5. ``check_entity_ids`` accepts exactly the arrays that the storing variable's - dtype holds unchanged. So an identifier outside the range, or not a whole - number, raises ``ValueError`` rather than being stored as another number. +5. ``check_entity_ids`` passes exactly the arrays that the storing variable's + dtype holds unchanged, and gives an ``EntityIdWarning`` for any other. So + building a simulation warns when an identifier is outside the range or not + a whole number, and stays silent otherwise (this module turns the warning + into an error, so every test above checks that). +6. Identifiers outside the range still give the entity structure the tables + describe: the populations take them as given. The group tables are sorted by identifier, as ``build_from_ids`` numbers the benefit units and households in that order, and every group has a member. @@ -43,9 +47,20 @@ from policyengine_uk import Microsimulation from policyengine_uk.data import UKMultiYearDataset, UKSingleYearDataset from policyengine_uk.system import system -from policyengine_uk.utils.entity_ids import ENTITY_ID_VARIABLES, check_entity_ids +from policyengine_uk.utils.entity_ids import ( + ENTITY_ID_VARIABLES, + EntityIdWarning, + check_entity_ids, +) -pytestmark = pytest.mark.usefixtures("cloned_uk_tax_benefit_system") +pytestmark = [ + pytest.mark.usefixtures("cloned_uk_tax_benefit_system"), + # An identifier the simulation cannot store exactly warns. Outside the + # tests that expect it, the warning fails the test. + pytest.mark.filterwarnings( + "error::policyengine_uk.utils.entity_ids.EntityIdWarning" + ), +] YEAR = 2025 ID_MIN, ID_MAX = -(2**31), 2**31 - 1 @@ -419,91 +434,111 @@ def held_unchanged(values: np.ndarray, dtype) -> bool: @pytest.mark.parametrize("variable", ENTITY_ID_VARIABLES) @given(data=st.data()) @settings(max_examples=300, deadline=None) -def test_check_accepts_exactly_what_the_store_holds_unchanged(variable, data): +def test_check_warns_exactly_when_the_store_changes_a_value(variable, data): """Invariant 5, against the cast that stores the values.""" definition = system.variables[variable] values = data.draw(storable_values(variable)) - try: - check_entity_ids(definition, values) - accepted = True - except ValueError as error: - assert variable in str(error) - accepted = False - assert accepted == held_unchanged(values, definition.dtype), values + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + exact = check_entity_ids(definition, values) + ours = [w for w in caught if issubclass(w.category, EntityIdWarning)] + assert exact == held_unchanged(values, definition.dtype), values + assert len(ours) == (0 if exact else 1) + assert all(variable in str(w.message) for w in ours) def test_check_leaves_other_kinds_of_identifier_to_the_store(): """Text and other non-numeric identifiers are not checked here; policyengine-core reports those it cannot store.""" definition = system.variables["person_id"] - check_entity_ids(definition, np.array(["a", "b"])) - check_entity_ids(definition, np.array([True, False])) - check_entity_ids(definition, np.array([], dtype=np.float64)) + assert check_entity_ids(definition, np.array(["a", "b"])) + assert check_entity_ids(definition, np.array([True, False])) + assert check_entity_ids(definition, np.array([], dtype=np.float64)) OUT_OF_RANGE_IDS = [ID_MAX - 1, ID_MAX, ID_MAX + 1] +MEMBERSHIP_VARIABLES = { + "person": {"person_id"}, + "benunit": {"person_benunit_id", "benunit_id"}, + "household": {"person_household_id", "household_id"}, +} -@pytest.mark.parametrize( - "entity, variable", - [ - ("person", "person_id"), - ("benunit", "person_benunit_id"), - ("household", "person_household_id"), - ], -) +def warned_variables(build): + """Run ``build`` and return it with the variables it warned about.""" + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always") + result = build() + messages = [ + str(w.message) for w in caught if issubclass(w.category, EntityIdWarning) + ] + return result, {message.split()[0] for message in messages} + + +@pytest.mark.parametrize("entity", ["person", "benunit", "household"]) @pytest.mark.parametrize("id_dtype", ["int64", "float64"]) -def test_identifier_above_the_range_is_refused(entity, variable, id_dtype): - """An identifier one above the range is refused when the entities are - built, whichever entity it names.""" +def test_identifier_above_the_range_warns_and_keeps_the_structure(entity, id_dtype): + """Invariants 5 and 6 for an identifier one above the range, whichever + entity it names. Only the columns that hold it warn.""" ids = {name: [1, 2, 3] for name in ("person", "benunit", "household")} ids[entity] = OUT_OF_RANGE_IDS person, benunit, household = tables( ids["person"], ids["benunit"], ids["household"], id_dtype=id_dtype ) - with pytest.raises(ValueError, match=f"{variable} ranges from .* to {ID_MAX + 1}"): - simulate(person, benunit, household) + sim, warned = warned_variables(lambda: simulate(person, benunit, household)) + assert warned == MEMBERSHIP_VARIABLES[entity] + assert_structure_matches_tables(sim, person, benunit, household) -def test_identifier_below_the_range_is_refused(): +def test_identifier_below_the_range_warns_and_keeps_the_structure(): ids = [ID_MIN - 1, ID_MIN, ID_MIN + 1] - with pytest.raises(ValueError, match=f"person_id ranges from {ID_MIN - 1} to"): - simulate(*tables(ids, ids, ids)) + person, benunit, household = tables(ids, ids, ids) + sim, warned = warned_variables(lambda: simulate(person, benunit, household)) + assert warned == set(ENTITY_ID_VARIABLES) + assert_structure_matches_tables(sim, person, benunit, household) -def test_identifier_that_is_not_a_whole_number_is_refused(): +def test_identifier_that_is_not_a_whole_number_warns(): person, benunit, household = tables([1, 2, 3], [1, 2, 3], [1, 2, 3], "float64") person["person_household_id"] = [1.0, 2.5, 3.0] - with pytest.raises(ValueError, match="person_household_id must hold whole"): + with pytest.warns(EntityIdWarning, match="person_household_id should hold whole"): simulate(person, benunit, household) -def test_single_year_dataset_refuses_an_identifier_out_of_range(): +def test_single_year_dataset_warns_about_an_identifier_out_of_range(): person, benunit, household = tables( OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS ) dataset = UKSingleYearDataset( - person=person, benunit=benunit, household=household, fiscal_year=YEAR + person=person.copy(), + benunit=benunit.copy(), + household=household.copy(), + fiscal_year=YEAR, ) - with pytest.raises(ValueError, match="person_id ranges from"): - Microsimulation(dataset=dataset) + sim, warned = warned_variables(lambda: Microsimulation(dataset=dataset)) + assert warned == set(ENTITY_ID_VARIABLES) + assert_structure_matches_tables(sim, person, benunit, household) -def test_core_dataset_file_refuses_an_identifier_out_of_range(tmp_path): +def test_core_dataset_file_warns_about_an_identifier_out_of_range(tmp_path): + """The rebuild from the export reads the stored identifiers, which differ + from the given ones, but each household keeps its one person.""" person, benunit, household = tables( OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS ) path = write_core_dataset_file(tmp_path / "dataset.h5", person, benunit, household) - with pytest.raises(ValueError, match="person_id ranges from"): - Microsimulation(dataset=path) + sim, warned = warned_variables(lambda: Microsimulation(dataset=path)) + assert warned == set(ENTITY_ID_VARIABLES) + for entity in ("benunit", "household"): + members = sim.populations[entity].members_entity_id + assert np.bincount(members, minlength=3).tolist() == [1, 1, 1] -def test_frame_refuses_an_identifier_out_of_range(): +def test_frame_warns_about_an_identifier_out_of_range(): frame = pd.DataFrame( - { - f"{variable}__{YEAR}": OUT_OF_RANGE_IDS - for variable in (*ENTITY_ID_VARIABLES, "age") - } - ) - with pytest.raises(ValueError, match="person_id ranges from"): - Microsimulation(dataset=frame) + {f"{variable}__{YEAR}": OUT_OF_RANGE_IDS for variable in ENTITY_ID_VARIABLES} + ).assign(**{f"age__{YEAR}": 40}) + sim, warned = warned_variables(lambda: Microsimulation(dataset=frame)) + assert warned == set(ENTITY_ID_VARIABLES) + assert sim.populations["household"].ids.tolist() == OUT_OF_RANGE_IDS + assert sim.populations["household"].members_entity_id.tolist() == [0, 1, 2] diff --git a/policyengine_uk/utils/entity_ids.py b/policyengine_uk/utils/entity_ids.py index c548e8b2b6..8c1d291292 100644 --- a/policyengine_uk/utils/entity_ids.py +++ b/policyengine_uk/utils/entity_ids.py @@ -1,19 +1,24 @@ """Entity identifiers a simulation can store exactly. A simulation built from data keeps each identifier column in two places. The -populations hold the entity structure (``ids`` and ``members_entity_id``). -The variables in ``ENTITY_ID_VARIABLES`` hold the same identifiers as stored -values, and those are what ``calculate``, ``to_input_dataframe`` and +populations hold the entity structure (``ids`` and ``members_entity_id``), +taken from the arrays ``build_from_ids`` is given. The variables in +``ENTITY_ID_VARIABLES`` hold the same identifiers as stored values, and those +are what ``calculate``, ``to_input_dataframe`` and ``UKSingleYearDataset.from_simulation`` return. ``Simulation.build_from_dataset`` rebuilds the entities from that last export. policyengine-core casts a stored value to its variable's dtype with ``astype`` -(``Holder._to_array``). That cast wraps an integer outside the dtype's range and -drops the fraction of a float, both without an error. ``check_entity_ids`` -refuses such identifiers when the entities are built, so that every identifier -a simulation accepts reads back exactly. +(``Holder._to_array``). That cast wraps an integer outside the dtype's range +and drops the fraction of a float, both without an error. ``check_entity_ids`` +warns about such identifiers when the entities are built. It does not refuse +them: the populations built from them still hold them exactly, and data builds +that number clones past the range (by adding multiples of a power of ten to +every identifier) load them today. """ +import warnings + import numpy as np from policyengine_core.variables import Variable @@ -28,8 +33,12 @@ ) -def check_entity_ids(variable: Variable, values) -> None: - """Raise ``ValueError`` if ``variable`` cannot store ``values`` exactly. +class EntityIdWarning(UserWarning): + """An identifier changes when the simulation stores it.""" + + +def check_entity_ids(variable: Variable, values) -> bool: + """Warn if ``variable`` cannot store ``values`` exactly. Only integer and float arrays are checked against an integer variable. policyengine-core reports any other mismatch when it stores the values. @@ -37,28 +46,43 @@ def check_entity_ids(variable: Variable, values) -> None: Args: variable: The variable that will store the identifiers. values: The identifiers, one per record. + + Returns: + Whether every value is stored exactly. A ``False`` result comes with + an ``EntityIdWarning``. """ values = np.asarray(values) if values.size == 0 or values.dtype.kind not in "iuf": - return + return True if not np.issubdtype(variable.dtype, np.integer): - return + return True if values.dtype.kind == "f": whole = np.isfinite(values) & (values == np.trunc(values)) if not whole.all(): - raise ValueError( - f"{variable.name} must hold whole numbers, but " - f"{int((~whole).sum())} of its {values.size} values are not " - f"(the first is {values[~whole][0]})." + warnings.warn( + f"{variable.name} should hold whole numbers, but " + f"{int((~whole).sum())} of its {values.size} values do not (the " + f"first is {values[~whole][0]}). The simulation stores them " + f"as {np.dtype(variable.dtype).name}, so the {variable.name} " + "values it returns will differ from them.", + EntityIdWarning, + stacklevel=3, ) + return False # Python integers compare exactly. A float32 or unsigned 64-bit bound does # not compare exactly with a NumPy integer limit. lowest, highest = int(values.min()), int(values.max()) limits = np.iinfo(variable.dtype) if lowest < limits.min or highest > limits.max: - raise ValueError( - f"{variable.name} ranges from {lowest} to {highest}, but a " + warnings.warn( + f"{variable.name} ranges from {lowest} to {highest}, but the " f"simulation stores it as {np.dtype(variable.dtype).name}, which " - f"holds whole numbers from {limits.min} to {limits.max}. Renumber " - "the identifiers to fit that range." + f"holds whole numbers from {limits.min} to {limits.max}. The " + f"{variable.name} values it returns will differ from the ones " + "given, and so may anything rebuilt from them. Renumber the " + "identifiers to fit that range.", + EntityIdWarning, + stacklevel=3, ) + return False + return True From 2d465a9642171314ad7a24203b444982f53036e3 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sat, 10 Oct 2026 18:11:54 -0400 Subject: [PATCH 3/4] Check object identifier arrays and narrow the out-of-range invariant From the review of #2273: - An object array of Python numbers, such as a pandas column mixing integers and floats, skipped the check, and core's astype truncates its fractions. check_entity_ids now converts such an array to the NumPy type that holds its values and checks that. Text and integers beyond 64 bits stay object and are left to core, which converts or rejects them. - The structure invariant for out-of-range identifiers covers UK tables and DataFrames only. build_from_dataset rebuilds a policyengine-core dataset file from the stored identifiers, which have changed and lost their order, so its test now checks only the warning. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/tests/test_entity_ids.py | 49 ++++++++++++++++++------ policyengine_uk/utils/entity_ids.py | 10 ++++- 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/policyengine_uk/tests/test_entity_ids.py b/policyengine_uk/tests/test_entity_ids.py index d6fcb59e63..5a3fcbac97 100644 --- a/policyengine_uk/tests/test_entity_ids.py +++ b/policyengine_uk/tests/test_entity_ids.py @@ -28,8 +28,11 @@ building a simulation warns when an identifier is outside the range or not a whole number, and stays silent otherwise (this module turns the warning into an error, so every test above checks that). -6. Identifiers outside the range still give the entity structure the tables - describe: the populations take them as given. +6. Identifiers outside the range, given in UK dataset tables or a + DataFrame, still give the entity structure the tables describe: the + populations take them as given. A policyengine-core dataset file is not + covered: ``build_from_dataset`` rebuilds it from the stored identifiers, + which differ from the given ones, so only the warning is checked there. The group tables are sorted by identifier, as ``build_from_ids`` numbers the benefit units and households in that order, and every group has a member. @@ -410,6 +413,17 @@ def floats(dtype): max_size=6, ).map(lambda values: np.array(values, dtype=dtype)) + # A pandas column that mixes integers and floats holds Python numbers. + python_numbers = st.lists( + st.one_of( + near_limits, + st.integers(-(2**63), 2**63 - 1), + st.floats(min_value=-(2.0**33), max_value=2.0**33), + st.floats(allow_nan=True, allow_infinity=True), + ), + max_size=6, + ).map(lambda values: np.array(values, dtype=object)) + return st.one_of( integers(np.int64), integers(np.int32), @@ -417,16 +431,20 @@ def floats(dtype): integers(np.uint64), floats(np.float64), floats(np.float32), + python_numbers, ) def held_unchanged(values: np.ndarray, dtype) -> bool: """Whether policyengine-core's cast to the variable's dtype - (``Holder._to_array``) keeps every value.""" + (``Holder._to_array``) keeps every value. A cast that fails keeps none.""" with warnings.catch_warnings(): # NumPy warns when a float is outside the integer range. warnings.simplefilter("ignore", RuntimeWarning) - cast = values.astype(dtype) + try: + cast = values.astype(dtype) + except (OverflowError, ValueError): + return False # Python compares integers with floats exactly; NaN equals nothing. return cast.tolist() == values.tolist() @@ -448,12 +466,22 @@ def test_check_warns_exactly_when_the_store_changes_a_value(variable, data): def test_check_leaves_other_kinds_of_identifier_to_the_store(): - """Text and other non-numeric identifiers are not checked here; - policyengine-core reports those it cannot store.""" + """Text and other non-numeric identifiers are not checked here; they are + left to policyengine-core's own conversion.""" definition = system.variables["person_id"] assert check_entity_ids(definition, np.array(["a", "b"])) assert check_entity_ids(definition, np.array([True, False])) assert check_entity_ids(definition, np.array([], dtype=np.float64)) + assert check_entity_ids(definition, np.array(["a", 1], dtype=object)) + + +def test_object_column_with_a_fraction_warns(): + """A pandas column that mixes integers and floats is an object array, + which core's cast would truncate.""" + person, benunit, household = tables([1, 2, 3], [1, 2, 3], [1, 2, 3]) + person["person_household_id"] = pd.Series([1, 2.5, 3], dtype=object) + with pytest.warns(EntityIdWarning, match="person_household_id should hold whole"): + simulate(person, benunit, household) OUT_OF_RANGE_IDS = [ID_MAX - 1, ID_MAX, ID_MAX + 1] @@ -522,16 +550,15 @@ def test_single_year_dataset_warns_about_an_identifier_out_of_range(): def test_core_dataset_file_warns_about_an_identifier_out_of_range(tmp_path): """The rebuild from the export reads the stored identifiers, which differ - from the given ones, but each household keeps its one person.""" + from the given ones and are no longer in order, so group inputs can land + on other groups (invariant 6 does not cover this path). The first build + warns about every column.""" person, benunit, household = tables( OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS, OUT_OF_RANGE_IDS ) path = write_core_dataset_file(tmp_path / "dataset.h5", person, benunit, household) - sim, warned = warned_variables(lambda: Microsimulation(dataset=path)) + _, warned = warned_variables(lambda: Microsimulation(dataset=path)) assert warned == set(ENTITY_ID_VARIABLES) - for entity in ("benunit", "household"): - members = sim.populations[entity].members_entity_id - assert np.bincount(members, minlength=3).tolist() == [1, 1, 1] def test_frame_warns_about_an_identifier_out_of_range(): diff --git a/policyengine_uk/utils/entity_ids.py b/policyengine_uk/utils/entity_ids.py index 8c1d291292..4bf4b1ed2f 100644 --- a/policyengine_uk/utils/entity_ids.py +++ b/policyengine_uk/utils/entity_ids.py @@ -40,8 +40,10 @@ class EntityIdWarning(UserWarning): def check_entity_ids(variable: Variable, values) -> bool: """Warn if ``variable`` cannot store ``values`` exactly. - Only integer and float arrays are checked against an integer variable. - policyengine-core reports any other mismatch when it stores the values. + Integer and float arrays are checked against an integer variable, and so + are object arrays of numbers, such as a pandas column that mixes integers + and floats. Anything else, such as text, is left to policyengine-core's + own conversion when it stores the values. Args: variable: The variable that will store the identifiers. @@ -52,6 +54,10 @@ def check_entity_ids(variable: Variable, values) -> bool: an ``EntityIdWarning``. """ values = np.asarray(values) + if values.dtype.kind == "O": + # Python numbers in an object array take the NumPy type that holds + # them all; core's ``astype`` would otherwise truncate a float. + values = np.array(values.tolist()) if values.size == 0 or values.dtype.kind not in "iuf": return True if not np.issubdtype(variable.dtype, np.integer): From bef1bae7b6452916eaa33402ccd9d9555028882d Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Sun, 11 Oct 2026 09:16:12 -0400 Subject: [PATCH 4/4] Leave nested object cells to core and state what the check covers From the second review of #2273: - An object array whose cells NumPy cannot line up, such as a nested array, made check_entity_ids raise. It now leaves such arrays to core. - The docstring and invariant 5 now name what is checked: integer and float arrays, and object arrays of Python floats and of integers within 64 bits. Decimal values and larger integers are left to core. - The property tests suppress Hypothesis's too_slow health check, which a slow runner tripped while drawing one input. Co-Authored-By: Claude Opus 5.5 --- policyengine_uk/tests/test_entity_ids.py | 17 +++++++++++------ policyengine_uk/utils/entity_ids.py | 18 ++++++++++++------ 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/policyengine_uk/tests/test_entity_ids.py b/policyengine_uk/tests/test_entity_ids.py index 5a3fcbac97..e0097f2319 100644 --- a/policyengine_uk/tests/test_entity_ids.py +++ b/policyengine_uk/tests/test_entity_ids.py @@ -23,8 +23,10 @@ the size of the identifiers. 4. A simulation built from a policyengine-core dataset file, which is rebuilt from its own export, has that same structure and those same identifiers. -5. ``check_entity_ids`` passes exactly the arrays that the storing variable's - dtype holds unchanged, and gives an ``EntityIdWarning`` for any other. So +5. For integer and float arrays, and object arrays of Python floats and of + integers within 64 bits, ``check_entity_ids`` passes exactly the arrays + that the storing variable's dtype holds unchanged, and gives an + ``EntityIdWarning`` for any other, including those whose cast fails. So building a simulation warns when an identifier is outside the range or not a whole number, and stays silent otherwise (this module turns the warning into an error, so every test above checks that). @@ -44,7 +46,7 @@ import numpy as np import pandas as pd import pytest -from hypothesis import example, given, settings +from hypothesis import HealthCheck, example, given, settings from hypothesis import strategies as st from policyengine_uk import Microsimulation @@ -280,7 +282,7 @@ def test_identifier_variables_are_stored_as_integers(variable): ), id_dtype="int64", ) -@settings(max_examples=30, deadline=None) +@settings(max_examples=30, deadline=None, suppress_health_check=[HealthCheck.too_slow]) def test_identifiers_in_range_read_back_exactly(members, id_dtype): """Invariants 1 to 3 for a simulation built from entity tables.""" person, benunit, household = tables(*members, id_dtype=id_dtype) @@ -292,7 +294,7 @@ def test_identifiers_in_range_read_back_exactly(members, id_dtype): @given(members=memberships(), id_dtype=st.sampled_from(["int64", "float64"])) @example(members=(ISSUE_IDS, ISSUE_IDS, ISSUE_IDS), id_dtype="float64") -@settings(max_examples=10, deadline=None) +@settings(max_examples=10, deadline=None, suppress_health_check=[HealthCheck.too_slow]) def test_core_dataset_file_builds_the_tables_structure( members, id_dtype, tmp_path_factory ): @@ -451,7 +453,7 @@ def held_unchanged(values: np.ndarray, dtype) -> bool: @pytest.mark.parametrize("variable", ENTITY_ID_VARIABLES) @given(data=st.data()) -@settings(max_examples=300, deadline=None) +@settings(max_examples=300, deadline=None, suppress_health_check=[HealthCheck.too_slow]) def test_check_warns_exactly_when_the_store_changes_a_value(variable, data): """Invariant 5, against the cast that stores the values.""" definition = system.variables[variable] @@ -473,6 +475,9 @@ def test_check_leaves_other_kinds_of_identifier_to_the_store(): assert check_entity_ids(definition, np.array([True, False])) assert check_entity_ids(definition, np.array([], dtype=np.float64)) assert check_entity_ids(definition, np.array(["a", 1], dtype=object)) + nested = np.empty(2, dtype=object) + nested[:] = [1, np.array([2])] + assert check_entity_ids(definition, nested) def test_object_column_with_a_fraction_warns(): diff --git a/policyengine_uk/utils/entity_ids.py b/policyengine_uk/utils/entity_ids.py index 4bf4b1ed2f..87df514ac0 100644 --- a/policyengine_uk/utils/entity_ids.py +++ b/policyengine_uk/utils/entity_ids.py @@ -41,9 +41,10 @@ def check_entity_ids(variable: Variable, values) -> bool: """Warn if ``variable`` cannot store ``values`` exactly. Integer and float arrays are checked against an integer variable, and so - are object arrays of numbers, such as a pandas column that mixes integers - and floats. Anything else, such as text, is left to policyengine-core's - own conversion when it stores the values. + are object arrays of Python integers and floats, such as a pandas column + that mixes them. Anything else (text, ``Decimal`` values, integers too + large for 64 bits, nested arrays) is left to policyengine-core's own + conversion when it stores the values, which may truncate or reject them. Args: variable: The variable that will store the identifiers. @@ -55,9 +56,14 @@ def check_entity_ids(variable: Variable, values) -> bool: """ values = np.asarray(values) if values.dtype.kind == "O": - # Python numbers in an object array take the NumPy type that holds - # them all; core's ``astype`` would otherwise truncate a float. - values = np.array(values.tolist()) + # Python integers and floats in an object array take the NumPy type + # that holds them all; core's ``astype`` would otherwise truncate a + # float. Cells NumPy cannot line up, such as nested arrays, are left + # to core. + try: + values = np.array(values.tolist()) + except (TypeError, ValueError): + return True if values.size == 0 or values.dtype.kind not in "iuf": return True if not np.issubdtype(variable.dtype, np.integer):