Repository navigation
Store person_household_id and person_benunit_id as integers - #2273
Merged
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This was referenced Oct 10, 2026
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This was referenced Oct 13, 2026
Collaborator
Author
|
Infra backlog hand-off (23 October 2026).
|
Contributor
Aggregate impact on certified data: passBase Checks: 0 broken by this change, 1 failing on both sides, 0 fixed. Moves flagged for review: 0 (thresholds in Notes
Moves flagged for reviewNone. Spending by benefit and service
Caseloads
Standard reform costings (change in gov_balance)
Standard reforms: change in each reform's target
Household net income by decileNo household's net income changes in any year. No change in any year: receipts by tax head; government balance, each component counted once; model totals; household income totals; population; poverty rates; people in poverty; inequality; mean household net income in each side's own deciles. Checks failing on both base and head (1)
Counted-once checkEach of 85 components was raised by £1 per unit in 2026-27, and every head of its totals was read household by household: its own head must rise by exactly £1, no other tax head may move, no other benefit may rise, and the model's own total must move by what its heads moved. A head that breaks a rule is measured again twice: with every other head held at its baseline value, and with every other head held at its value after the first £1 while the component rises again. It fails where either still shows the response. 0 failed; 0 were skipped.
Provenance
Dataset: Receipts by tax head: head levels
Spending by benefit and service: head levels
Caseloads: head levels
Government balance, each component counted once: head levels
Model totals: head levels
Household income totals: head levels
Population: head levels
Poverty rates: head levels
People in poverty: head levels
Inequality: head levels
Mean household net income in each side's own deciles: head levels
Standard reform costings (change in gov_balance): head levels
Standard reforms: change in each reform's target: head levels
|
Keep integer membership expectations from newer main and add a deterministic large-ID example for both group types. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
marked this pull request as ready for review
October 23, 2026 19:40
juaristi22
approved these changes
Oct 24, 2026
juaristi22
left a comment
juaristi22
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at 4d85ed08be16152ed699ee9c9b620fe43e961ab3. No actionable findings in the identifier storage, build warning or export/reload changes.
The two person membership variables now use integer storage consistent with the entity IDs they reference. The warning checks match the actual Core dtype and handle fractional/non-finite values before casting. The documented int32 range remains; this does not promise int64 storage.
Validation: 16 focused tests passed, including five 300-example Core-casting properties, the high-ID reproduction across projected years, H5 membership rebuild, and changed CSV/export/reload checks with income-tax equality. Ruff formatting and all current-head CI checks pass. No representative-population invariance claim is made.
Collaborator
Author
|
Merged by the UK hub (Claude session 8e19f9d5) at head
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2267
person_household_idandperson_benunit_idnow use the same integer storage as the other entity identifiers. IDs such as 900,000,001, 900,000,002 and 900,000,003 remain distinct when calculated, exported, saved as CSV and loaded again. The old float32 memberships rounded these IDs together and could merge households during a Core-dataset export/rebuild.The variables use core's existing int32 storage. Within -2,147,483,648 through 2,147,483,647, whole-number identifiers remain exact. Exported membership columns are integers; pandas reads the CSV integer text as int64. This PR does not widen core's store to int64. The five-ID check warns if a value cannot fit its storing variable exactly, retaining the existing behavior for builds using identifiers outside that range.
Merged current main with a merge commit, preserving its construction-reform state handling and group-row alignment. The two failing dataframe round-trip tests now require exact integer memberships for both households and benefit units, verify CSV int64 values, and retain the existing structure/value checks. Legacy float input compatibility tests remain.
simulation.pyhas only the original PR's 21 added lines; no extra production logic was added during adoption.Validation uses a local
UV_LINK_MODE=clone uv sync --frozen --extra devPython 3.13 environment (core 3.33.2), foregroundpytest -n 2, two BLAS/OpenMP threads, and the required load/disk/compressor gates. 33 focused tests passed (300 existing MicroSeries array-access warnings, 2626.95s), including all 30 identifier cases and the three dataframe/reform round trips. The full-input Budget differential passed: 108 financial arrays, 108 weighted totals, 36 weight arrays and six entity-structure checks match exactly. Final head and detailed scope are recorded in the hand-off comment.make format,uvx ruff format --check .and the F811/F821 package checks pass.Budget scope: the managed route converts to
UKSingleYearDatasetand calls countryMicrosimulation; publication calculators consume its arrays/weights directly, bypassing the wrapper’s dataframe membership joins. No UK policy formula reads either changed membership variable. The numerical differential checks nine monetary variables for 2026–2031 under current law and a dated before-load allowance reform; it does not regenerate every poverty/decile/student-loan/publication output.The remaining range limitation is deliberate: identifiers outside int32 warn and may be altered when stored. Existing follow-ups are policyengine-core#467, policyengine-uk-data#558 and microcosm#1194. Main has already fixed the older group-table ordering problem (#2278), so that former PR precondition no longer applies.
Reproducer: policyengine_uk/tests/test_dataframe_dataset.py::test_an_exported_frame_saved_as_csv_loads_back
axiom: n/a: identifier storage and simulation export infrastructure
Independent review and landing remain with the hub.