Repository navigation
Conversation
Simulation.subsample was inherited from policyengine-core and raised TypeError: it rebuilds through build_from_dataset() with no arguments, and the UK builder takes the dataset to load. Making that argument optional would not have been enough. Core's method reads each person's household and benefit unit back from the stored person_household_id and person_benunit_id inputs, which the UK model holds as 32-bit floats, so identifiers above 2**24 would merge households. It also leaves Simulation.dataset as a core Dataset, while UK formulas read dataset.years, and it rebuilds a baseline branch, while a UK baseline is a simulation of its own. The UK method takes households, benefit units and people from the populations, keeps every supplied input for the sampled records, rescales each supplied weight to keep its total, and subsamples the baseline and the dataset with the same draw. Fixes #2254 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A result calculated before subsampling is calculated again on the sample, and the record of an input that a calculated value replaced is not kept. Found by mutating the subsample module: removing the cache invalidation went unnoticed until these assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The simulations usage page gains a short section on subsampling, since the method now works for UK simulations. A simulation built from a DataFrame keeps no dataset object, so test that subsampling it leaves dataset as None and still cuts the baseline to the same households. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CI formats Python blocks in Markdown with the latest ruff, which the locked ruff does not check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge-order note from #2273 (fixes #2267, stores On a local merge of this branch ( assert len(set(stored.tolist())) < len(households)With #2273 the stored values tell the 900-million households apart ( Whichever PR lands second should:
🤖 Generated with Claude Code |
|
UK hub, for this PR's owner: test interaction with #2270. #2270 (DataFrame inputs load as the dataset they describe) is armed to land once its
This PR's last |
Address the independent review of #2265: - Subsampling a clone changed the source simulation's baseline, because policyengine-core's clone shares a separate baseline. Replace each baseline with a subsampled copy instead of changing it in place. - A calculated value cached in memory over an input stored on disk was kept as the input. Read each supplied input from the tier the holder records it in, as Holder._stores_user_input decides. - A trace kept values calculated on the replaced populations. Give each simulation a new tracer, shared as before. - Say that dataset holds the rows of the dataset the simulation was built from, without later set_input values, and that weights are unbiased in the sampling period only. Generate multi-year datasets whose weights change differently by household in the property test. Also adapt to main: a DataFrame-built simulation now has a dataset, and test memberships without assuming how the identifiers are stored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Address the second independent review of #2265: - Copying a baseline with clone copied every cached array before the subsample discarded them, which took a full-size run past a 20 GB memory limit. Announce the copy as get_branch does, so it shares the arrays as views and copies none. - A negative household weight could leave a negative weight in the sample. Refuse negative sampling weights, and negative values in any weight being rescaled. - An input for part of a year, which a DataFrame can give, stays in the simulation and out of the dataset of whole years. Say so, and test it. - Give the property test two explicit cases whose weight shares change from year to year, which the generated cases had not reached. - Qualify the docstring's sampling and unbiasedness claims by quantize_weights, and describe the first-weights fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mutation testing showed the rescaling check alone also caught a negative household weight, so removing the draw's own check went unnoticed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
For coordination: #2308 (fixes #2307) is stacked on this branch and builds on
Nothing in this PR needs to change for #2308. If |
|
CI run cancelled to free runners (UK hub, 2026-10-11 23:30Z). The PolicyEngine org is at GitHub's limit of 60 concurrent jobs, with about 360 queued. Fleet ops asked the hub to let the Budget-critical #2305 take runners first. This PR's queued run had not started, so I cancelled it. Nothing is wrong with the PR. Please don't re-run it or push just to retrigger CI. The hub re-runs this run itself once #2305 has runners. The record is |
vahid-ahmadi
left a comment
There was a problem hiding this comment.
Fine once it is merged with main and keeps the model-value record across the rebuild.
@MaxGhenis, the approach is right. The memberships come from the populations, the baseline and the dataset are cut with the same draw, and the draw is deterministic and independent of process. The quantized weights are unbiased in the sampling year: E[count] = n·w/W, so E[count·W/n] = w. Two things need doing before merge, both from main moving on since 82e0c4995.
- must: merge main.
policyengine_uk/utils/supplied_inputs.pyconflicts in two places, the module docstring and the end of the file. Both sides add text there, so keeping both works: I did that locally and the import is fine. Runruff formatafterwards, because the joined file gets an extra blank line. - must, after the merge: subsample turns a scenario's model value into a supplied input. Main now has
set_model_value(supplied_inputs.py), andscenarios/uc_reform.py:163uses it foruc_LCWRA_element._apply(utils/subsample.py:336) callsbuild_from_ids, which on main callsbuild_from_populations, which callsforget_model_values. The loop atsubsample.py:353then stores every kept value withset_input, which main treats as supplied. On a local merge, with the six-householddataset()fromtest_subsample.py, I calledset_model_value(sim, "uc_LCWRA_element", 2026, ...)and thensubsample(n=4, seed=0, time_period=2026).supplied_input(benunit, "uc_LCWRA_element", 2026)wasNonebefore the subsample and returned the stored array after it._model_valueswent from four keys to none. So a subsampled scenario simulation reads its own adjustment as a user's entry. That breaks the "same results as the same households in the full simulation" property for formulas that asksupplied_input. Suggested fix: in_plan, cut each_model_valuesarray by the same entity positions; in_apply, write the cut arrays back intosimulation._model_valuesafter theset_inputloop. Add a test thatsupplied_inputis stillNoneafter the subsample. - nit:
rescale_weights(subsample.py:139). Withquantize_weights=True, a record whose own weight is 0 gets a positive weight if its household was drawn:rescale_weights([0, 100, 100], [1, 1, 0])gives[100, 100, 0]. In the sampling year this can't happen for households. It can for a household with zero weight in another year, or for a supplied person or benefit unit weight of 0. If that is intended, say so in the docstring. If not, usecounts * (weights > 0). - nit: each supplied weight is rescaled to its own entity's total separately (
entity_countsin_plan). If a dataset ever suppliesperson_weightalongsidehousehold_weight, a person's new weight will not equal their household's, though the model defines it that way. None of the dataset code here supplies it today, so a line in the docstring is enough.
What I checked
- Fresh uv venv at
82e0c4995. I then mergedorigin/mainlocally (the merge is not pushed), resolved the conflict as in point 1 and re-synced to the merged lock. - On that merge:
test_subsample.py,test_input_records.py,test_simulation_dataset_sources.pyandtest_dataframe_dataset.pygave 141 passed. They don't cover point 2. - The model-value probe in point 2, and
rescale_weightscalled directly for point 3. ruff checkandruff format --checkclean onutils/subsample.py,simulation.pyandtests/test_subsample.py.- CI at
82e0c4995: Test, Lint, Fix evidence and the smoke imports passed. There is no run on a merge with current main, because GitHub reports the PR as conflicting. - Not checked: the full-size dataset run and the memory figures in the PR body. My #2193 adds
apply_reformtosimulation.py, in a different place fromsubsample, so there is no textual clash.
🤖 Generated with Claude Code
|
A note from #2419, the CGT retention-rate default, tracked in the checklist on PolicyEngine/policyengine.py#602. Once #2419 lands, every reform simulation measures marginal rates on gains against its baseline, at the reform's own gains before the response. So the baseline has to hold the same people. A |
|
UK hub takeover for the Budget fix sprint (Max, 21 Oct). The owner session If the owner session wakes, please don't push to this branch; comment here instead, so there's only one writer. 🤖 Generated with Claude Code |
|
UK hub: takeover withdrawn. The owner session has been woken (22 Oct 01:25Z) and is finishing this PR itself, so the hub's build is cancelled. One writer: the owner session. 🤖 Generated with Claude Code |
|
The owner session has resumed this PR at the UK hub's request (2026-10-22 01:20Z); the hub's takeover note above is withdrawn and its build |
Main added a record of the model's own values (set_model_value), which the UK Universal Credit health element reform stores in every simulation, and a record of where each capital gains input came from. A subsample rebuilds the populations, which forgets both, so it now stores each model value back with set_model_value, restores the capital gains record, and carries the dataset's projection annotation. Resolved the supplied_inputs.py conflict by keeping both sides' helpers, and the simulation.py import conflict by importing both modules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…copies Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 84 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
|
Address vahid-ahmadi's review of #2265 (points 3 and 4): - With quantize_weights, a drawn record whose own weight is zero got a positive weight: a household drawn by its weight in the sampling year with a zero weight in another year, or a zero person or benefit unit weight. A zero weight stands for no one, so it now stays zero, and the year's total goes to the drawn records that stand for someone. - Each supplied weight is rescaled to its own entity's total, so a person weight supplied alongside household weights need not equal the new weight of the person's household. Say so where the weights are listed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d has no weight Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@vahid-ahmadi thank you; all four points are addressed.
Since then: the targeted suite (subsample, input records, dataset sources, DataFrame round trip, the UC health element tests, CGT projection and response default) gave 377 passed on core 3.33.2 and on 3.33.12 at |
|
@juaristi22 thanks for the note on #2419. |
|
Hand-off to the UK hub (Budget fix sprint). The owner session
|
|
UK hub independent review (GPT-6.1 Sol) at db3a38e: REQUEST_CHANGES, one P2. Subsampling drops stored inputs for neutralised variables ( Everything else checked out: all four of Vahid's points, memberships and ids, storage tiers, baseline isolation, UC model values, CGT provenance; 🤖 Generated with Claude Code |
|
UK hub hand-off for review P2 at comment 6335788910: fix committed locally; validation and push blocked by host limits.
|
|
UK hub (Claude session, for Max): independent review r2 (GPT-6.1 Sol) at [P2] Restoring a neutralized variable still returns zero after subsampling — Everything else in the round-one fix holds: canonical stored inputs, supplied/model ownership and ordinary replay. CI at this head fails exactly the two new regressions. |
|
UK hub (Claude session, for Max): independent review r3 (GPT-6.1 Sol) at
The underlying bug is in policyengine-core, where a |
|
UK hub (Claude session, for Max): waiting on policyengine-core. A holder that tracks the system on every read and write would also reject the unchanged CGT test's £555 write made while the variable is neutralised. So a correct fix needs core's Holder to follow runtime variable replacement. Four cases are marked A core fix is planned. Once it ships, bump core, remove the four markers and their two allowlist entries, and re-review. This isn't Budget work, so it stays out of the merge queue until then. |
Fixes #2254
Status: waiting for policyengine-core holder resync. The r3 invalidation workaround has been removed. Supplied/model restoration regressions remain strict expected failures until the core fix is released.
What was wrong
Simulation.subsample()was inherited from policyengine-core and raisedTypeError: Simulation.build_from_dataset() missing 1 required positional argument: 'dataset'. I reproduced it onmain(2.125.2) with a six-household in-memory dataset, on policyengine-core 3.32.27 (the locked version) and 3.33.1.Why the signature was not the whole problem
I read core's
subsampleand flat-file loader (master8fdf59e9, 3.33.2) against the UK builders, and ran the issue's option 3:build_from_dataset(self, dataset=None)falling back toself.dataset, core'ssubsamplehands the UK builder the flat-fileDatasetit rebuilt withDataset.from_dataframe. The UK builder callsdataset.load_dataset(), which reads only the table, time-period-array and array formats, and raisesUnboundLocalErrorfor a flat file. I ran this on core 3.32.27 and 3.33.2. The UK builder would also rebuild every year from the first one, projected forward, rather than keep each year's inputs.person_household_idandperson_benunit_idinputs. Onmainthe UK model stores both as 32-bit floats, which hold integers exactly only up to 2**24 (16,777,216). In the full-size UK dataset the issue's reproduction used, household identifiers reach about 900 million: its 535,080 distinct household identifiers are only 76,551 distinct values as float32, and its 618,980 benefit unit identifiers are 544,894. A rebuild from those columns would merge households and benefit units. (Store person_household_id and person_benunit_id as integers #2273 proposes storing them as integers. This PR does not depend on it.)branches["baseline"]. A UK simulation's baseline is a separateSimulation, which core's method does not touch, so a reform and its baseline would hold different households.seed, core readsself.dataset.name. UK datasets have noname.weight__. In the UK that would includehousehold_bus_fare_age_weightif it were supplied as an input; it shares out bus fare spending and is not a sampling weight.So this is option 1 in the issue: a UK
subsample. It does not call core's method and does not changebuild_from_dataset. The subsample implementation stays in the UK model. This PR waits for the policyengine-core holder resync fix. Holders cache variable instances after runtime reforms replace the system definitions. The attempted UK invalidation workaround has been removed: it caused core to reject an existing neutralised CGT input replacement, and it was too late for writes inside a restoring reform. Keeping holders current on every access cannot preserve the unchanged CGT contract through core’s public input API. The supplied/model restoration regressions remain strict expected failures with reasonneeds policyengine-core holder resync; every assertion is retained.What the UK method does
Same signature as core's:
subsample(n=None, frac=None, seed=None, time_period=None, quantize_weights=True).nhouseholds with replacement and keeps every household drawn at least once, with its benefit units and people. Only households with a positive weight intime_periodcan be drawn: withquantize_weights=True(the default), each with probability proportional to that weight; without it, each with equal probability. A period before the first one the simulation was given household weights for has none, so the first given weights are used.idsandmembers_entity_id), which are exact, not from the stored identifier inputs.set_input) for the sampled records, for every period. It reads each input from the storage tier (memory or disk) the holder records it in, so a value calculated later and cached over the input is not kept. Calculated values are dropped and recalculated on the sample, and the tracer starts again.household_weight,benunit_weight,person_weight,original_weight,raw_person_weight), and a test fails if a new variable ending inweightis not classified.cloneshares a separate baseline between a simulation and its clone, so subsampling a clone would otherwise cut down the source's baseline. The copy is made asget_branchmakes a branch, so it shares the old baseline's arrays as read-only views instead of copying them, and it rebuilds its own at once. (A plainclonecopied every cached array first, which took the full-size run below past a 20 GB memory limit.) The old baseline stays in memory until nothing holds it.datasetwith the rows of the dataset the simulation was built from for the sampled records, weights rescaled the same way. The original object is not changed (it can be the shared entry of the URL cache). Like the dataset before subsampling, it holds whole years only, and not values set withset_inputafter the build.mainadded since this PR opened, which a rebuild of the populations forgets. Values the model stored as its own withset_model_value(the Universal Credit health element reform stores them for every simulation, for 2026 to 2032) are stored back withset_model_value, sosupplied_inputstill does not read them as supplied. The record of where each capital gains input came from (supplied or projected, by period) is restored, and the dataset's annotation of projected capital gains inputs is carried to the subsampled dataset.Differences from core's method, all deliberate:
numpy.random.RandomStateseeded from a SHA-256 digest of the seed, so a seed draws the same households in every process, and the global random state is not touched. A seed therefore does not pick the same households as it would in core.ValueError.Invariants
Stated in
policyengine_uk/tests/test_subsample.pyand tested with Hypothesis for generated datasets of one year (which the model projects) and of several years whose weights change differently from household to household (two explicit examples make sure the shares change), for everyn, seed, sampling period andquantize_weights:nhouseholds are kept, each with a positive weight in the sampling period.n, with the multiples summing ton. The same draws set every year's weights. Withquantize_weights=False, it is proportional to its old weight times a whole number of draws. Either way, a zero weight stays zero in every year, and each supplied weight is rescaled to its own entity's total (so a supplied person weight need not equal the new weight of the person's household).datasetholds the same records as the populations. For a simulation whose inputs are all in its dataset, a simulation built from that dataset gives the same results as the subsampled simulation. This compares the two implementations of the subsample: the one that cuts the stored inputs and the one that cuts the dataset tables.Simulation._get_recorded_input_array(it skips on 3.32, which has no such method).capital_gains_projection_metadataare unchanged. A simulation built from the subsampled dataset reports the same origins as one built from the full dataset.Intended exceptions, each pinned by a test:
quantize_weights=Falsekeeps the total exactly, which makes the new weights a ratio estimate. I measured the bias: about 6% at 40 draws and under 1% at 400, for a seven-household example. This is the same definition as core's.dataset: one set withset_inputafter the build, or one for part of a year, which a DataFrame can give and a dataset of whole years cannot hold. A weight set withset_inputon the simulation alone is rescaled to its own total there, while the baseline and the dataset keep the dataset's weights.Tests
UK hub r3 follow-up / stop decision (2026-10-23): reverted the invalidation override to parent behaviour. New supplied/model regressions check writes inside restoration both directly and after subsampling; direct cases and the unchanged CGT test pass under parent behaviour, while the existing r2 restoration cases and sampled write cases require core holder resync. At
e4c54d7fa, the locked foregroundpytest -n 2run passed 244 tests with 4 strict expected failures, 0 unexpected failures and 0 skips in 1h12m40s: subsample (56 passed / 2 xfailed), input records (3 passed / 2 xfailed), the whole CGT projection file (115 passed), input DataFrame export (25 passed), reform-input lifecycle (17 passed), and reform representations (28 passed). The unchanged CGT routes and both direct supplied/model write controls passed. Touched-file Ruff format/lint, repository-wideuvx ruff format --check .(1940 files), whitespace and CI xfail-allowlist checks passed. This PR must wait for the upstream fix.UK hub r2 P2 fix (2026-10-23): existing holders rebind to the current system definitions before reform cache invalidation. Both supplied/model restoration regressions and the canonical neutralised-input replay remain unchanged. At
74b0c65d7, the locked foregroundpytest -n 2run passed 85 tests, 0 failed, 0 skipped in 47m10s:test_subsample.py(58),test_input_records.py(1),test_input_dataframe_export.py(25), andtest_capital_gains_projection.py::test_restoring_neutralized_projection_preserves_applied_fingerprint(1). Python 3.13.9, core 3.33.2. Touched-file Ruff format/lint, repository-wideuvx ruff format --check .(1940 files), and whitespace checks pass. No full suite or aggregate-impact run was repeated for this infrastructure fix.UK hub P2 follow-up (2026-10-22): commit
9a91736f3(on mergefc537e4e3oforigin/mainatfe0b99a36) preserves neutralised stored inputs and model markers when subsampling, and adds supplied/model ANI restoration regressions. Touched-file Ruff format and lint, whitespace checks and repository-wideuvx ruff format --check .pass. The focusedpytest -n 2runs were not run locally: the host was outside its load and disk limits (load1 172.65, free disk 54.98 GB). CI on this head is the test evidence, and the hub's independent review reruns the focused tests.policyengine_uk/tests/test_subsample.py: 58 tests. They cover the reported call, each failure mode above, each refusal, the findings of two review rounds and of @vahid-ahmadi's review, the recordsmainadded, and the properties.Mutation check at
01f5d051: I made 22 single changes toutils/subsample.pyandutils/supplied_inputs.pyand ran the file against each. 21 were caught. The survivor removes the reset of the input record, which_invalidate_all_cachesrepeats later in the same function, so it changes nothing observable. After mergingmain, three more (store model values withset_input, drop the capital gains origins, drop the dataset annotation) were each caught by the two new tests.Local runs at
7b4c3c28(the branch withmainat7918fc2amerged in), with 4 workers, of the new file plustest_input_records.py,test_simulation_dataset_sources.py,test_dataframe_dataset.py, the fourtest_uc_health_*.pyfiles,test_capital_gains_projection.pyandtest_capital_gains_response_default.py:At
ab843c80(zero weights stay zero),test_subsample.pywith 4 workers: core 3.33.2, 56 passed; core 3.33.12, 56 passed. Later commits change only docstrings.Full-size check under core 3.33.2 at
5dd27677, before the merge withmainthat added the model-value and capital gains handling (not rerun since), on the full-size UK dataset the issue's reproduction used (535,080 households):subsample(n=10_000, seed=0, time_period=2026)took 6.7 seconds and kept 8,660 households. From the weights alone, the expected number of distinct households in such a draw is 8,634. Memberships matched the dataset's 64-bit identifiers exactly, the 1,260 supplied variable-periods were the same as before, the baseline was a new copy with the same records and the old baseline still held every household, every year ofdatasetheld the same records, and each year's household weight total was unchanged to 3 parts in 100 million. The same held forquantize_weights=False(9,864 households, 4.0 seconds).Memory: that run stayed under the 20 GB process limit that the plain-
cloneversion exceeded. On a synthetic dataset of 60,000 households with 922 supplied variable-periods, the subsample raised the process's peak resident memory from 2,755 to 2,786 MiB (about 1%).Not run locally: the full suite. CI runs it.
Aggregate impact
From the Aggregate impact report on
7b4c3c28merged intomain(b4495e5cb1againsta8b2848c13,enhanced_frs_2024_25.h5, policyengine-uk-data 1.58.1, fiscal years 2025-26 to 2031-32):The one check failing on both sides is
sign:vat(VAT is never negative), which this PR does not touch. Expected:subsampleis not called in a full microsimulation, and the changes tosupplied_inputs.pyonly add helpers. Later commits change onlyutils/subsample.pyand its tests.Found on the way
Simulation(dataset=<DataFrame>)failed for households of more than one person. Fixed by Load a DataFrame simulation input as the dataset it describes #2270, merged; this branch includes it.person_household_idandperson_benunit_idare stored as 32-bit floats. Store person_household_id and person_benunit_id as integers #2273 is open. This PR reads memberships from the populations and does not depend on it.supplied_inputandsupplied_input_periodsreport a value calculated and cached over an input as supplied. Count only supplied inputs that are still stored in their tier #2308 is open and stacked on this branch: it builds onstored_supplied_valuefrom this PR.axiom: n/a: simulation infrastructure, no policy rule change
🤖 Generated with Claude Code
Reproducer:
policyengine_uk/tests/test_subsample.py::test_subsample_runs_on_a_dataset_simulation