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 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. |
|
Picking this up for the UK hub. Reply here if you're mid-change and I'll stop. Generated by Claude Code |
Conflicts: - simulation.py: keep both imports (measurement_branches from main, subsample from this branch). - utils/supplied_inputs.py: keep main's wording for what a rebuild forgets and this branch's paragraph on supplied_input_arrays. - tests/test_input_records.py: keep main's rebuilt-population tests and this branch's restoring-reform test; join the imports and use main's `period` import. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6HhDW9nQVQhdZKvrUS
main records each dataset year's ISA income contract and the series its savings income grows by on the simulation and the dataset. A rebuild of the populations resets the simulation's copies, and the subsampled dataset was built without them, so a legacy-contract dataset projected savings interest past its last year differently after a subsample. Restore both records after the rebuild, alongside the capital gains origins, and carry each year's energy price basis, ISA income contract and savings income series to the subsampled dataset. Also reword the subsample module docstring for integer membership ids, and rename the changelog fragment to 2254.fixed.md without list markers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6HhDW9nQVQhdZKvrUS
Removing the restore of the defaulted capital gains periods after a subsample changed the reported origin of a neutralised input from default to projected, and no test caught it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6HhDW9nQVQhdZKvrUS
|
Generated by Claude Code |
|
Generated by Claude Code |
|
Ready for hub review
Merge resolutions (
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #2254
Reproducer:
policyengine_uk/tests/test_subsample.py::test_subsample_runs_on_a_dataset_simulationaxiom: n/a: simulation infrastructure, no policy rule change
Status: waiting for policyengine-core holder resync (policyengine-core#644). Four supplied/model restoration regressions stay strict expected failures until core releases the fix.
mainnow locks policyengine-core 3.34.1, whoseHolderstill keeps theVariableit was built with, so the four cases still fail at44f6deb7(see Tests).What was wrong
UK simulations inherited
Simulation.subsample()from policyengine-core, and it 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 (then 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. Those hold whatever a dataset orset_inputstores there, not necessarily the populations' memberships. Onmainat 2.125.2 the UK model stored 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, the 535,080 distinct household identifiers came to only 76,551 distinct float32 values, and the 618,980 benefit unit identifiers to 544,894. Store person_household_id and person_benunit_id as integers #2273 has since moved both to 32-bit integers. The UK method reads memberships from the populations either way.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 a caller supplied it as an input; it shares out bus fare spending and does not weight a sample.So this is option 1 in the issue: a UK
subsample. It does not call core's method and does not changebuild_from_dataset. Separately, core's holders keep the variable they were built with after a runtime reform replaces its definition. A UK workaround that rebound holders when core invalidated caches made core reject an existing neutralised CGT input replacement and came too late for writes inside a restoring reform, so this branch does not carry it. The four supplied/model restoration regressions stay strict expected failures with reasonneeds policyengine-core holder resync, with every assertion kept.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 holds household weights for has none, so the method samples by the first weights it holds.idsandmembers_entity_id), which hold them exactly, 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 it does not keep a value the model calculated later and cached over the input. It drops calculated values, which the model recalculates on the sample, and starts the tracer again.WEIGHT_VARIABLESlists the weights (household_weight,benunit_weight,person_weight,original_weight,raw_person_weight), and a test fails if a new variable ending inweightappears in neither list.cloneshares a separate baseline between a simulation and its clone, so subsampling a clone would otherwise cut down the source's baseline. The method makes the copy asget_branchmakes a branch, so the copy shares the old baseline's arrays as read-only views instead of copying them, and 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. It leaves the original object as it was (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. Each year keeps the dataset's energy price basis, ISA income contract and savings income series.set_model_value(the Universal Credit health element reform stores them for every simulation, for 2026 to 2032), which it stores back withset_model_value, sosupplied_inputstill does not read them as supplied;set_inputrejects a neutralised variable;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 method leaves the global random state alone. A seed therefore does not pick the same households as it would in core.ValueError, a simulation built from a situation, one given no household weights or a negative weight, a branch, a benefit unit spread over two households, and a baseline holding other households.Invariants
policyengine_uk/tests/test_subsample.pystates these, and tests them 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, 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 stays proportional to its old weight times a whole number of draws. Either way, a zero weight stays zero in every year, and the method rescales each supplied weight 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 all come from 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_metadatastay the same, including the default origin of a neutralised input. A simulation built from the subsampled dataset reports the same origins as one built from the full dataset. The ISA income contract of every year, the savings income series and each year's energy price basis stay the same on the simulation, its baseline copy and the dataset, and savings interest past the dataset's last year matches the full simulation for the same people.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 atab843c80: about 6% at 40 draws and under 1% at 400, for a seven-household example. Core uses the same definition.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. The method rescales a weight set withset_inputon the simulation alone to its own total there, while the baseline and the dataset keep the dataset's weights.Merge with
main29504123mergesmainat47a2817e(2.175.2, policyengine-core 3.34.1). Three files conflicted:policyengine_uk/simulation.py: both sides added an import at the same place. The merge keepsmain'smeasurement_branchesimport and this branch'ssubsample_simulationimport.policyengine_uk/utils/supplied_inputs.py: both sides edited the module docstring's last paragraph. The merge keepsmain's wording for what a rebuild forgets ("when a rebuild installs populations that do not hold it") and this branch's paragraph onsupplied_input_arrays.policyengine_uk/tests/test_input_records.py: both sides added imports and tests. The merge keepsmain's ten rebuilt-population tests and this branch's restoring-reform test, joins the imports, and renames the test's localperiodtoinput_periodso that it can usemain'speriodimport.maindoes not do anything this branch does, so the merge drops nothing.mainadded two dataset records that the rebuild insubsampleresets: each year's ISA income contract and the savings income series. Without them, a subsample of a dataset under the legacy ISA contract projected savings interest in 2035 from the additive default (−110.29 instead of −100.00 for one person in the test).44f6deb7restores both on the simulation, and carries them and the energy price basis to each year of the subsampled dataset.Tests
mainat47a2817e: at44f6deb7, with the locked environment (policyengine-core 3.34.1) andpytest -n 4, 25 files gave 874 passed, 4 strict expected failures, 0 failed, 0 skipped in 43 minutes:test_subsample.py(57 passed / 2 xfailed),test_input_records.py(13 passed / 2 xfailed), every other file that callssubsampleor the supplied-input helpers, the CGT projection and response-default files, the four UC health element files, dataset sources, the DataFrame round trip, input DataFrame export, reform-input lifecycle, reform representations, the ISA income contract and the energy price basis files. The four expected failures are the restoration cases above.policyengine_uk/tests/code_healthgave 3,163 passed at44f6deb7. Ate804e8bc(which only adds a test), the two new tests pass. Ate804e8bc,ruff format --check .(2,229 files),ruff check .,ruff check --select F811,F821 policyengine_uk,git diff --checkagainstmain,.github/check_changelog_fragments.pyand.github/check_fix_pr.pyon this title and body pass.44f6deb7, each in a scratch copy, runningtest_how_the_dataset_says_its_amounts_grow_is_kept: without the restore of the ISA contracts and savings series, it fails on the contracts; without the dataset metadata, it fails on the subsampled dataset's ISA income contract; the unchanged copy passes.44f6deb7of the post-merge code, each in a scratch copy, runningtest_a_model_value_stays_the_modelsandtest_where_capital_gains_inputs_came_from_is_kept: dropping the capital gains origins, dropping the dataset's capital gains annotation and storing model values withset_inputeach fail one of them. Dropping the defaulted capital gains periods passed both, soe804e8bcaddstest_a_neutralised_capital_gains_input_still_reads_as_its_default, which fails with that mutation and passes without it.e4c54d7fa, the lockedpytest -n 2run passed 244 tests with 4 strict expected failures, 0 unexpected failures and 0 skips: 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).01f5d051: 22 single changes toutils/subsample.pyandutils/supplied_inputs.py, each run against the test file. The tests caught 21. The survivor removes the reset of the input record, which_invalidate_all_cachesrepeats later in the same function, so it changes nothing observable.7b4c3c28(mainat7918fc2amerged in), with 4 workers, the subsample, input records, dataset sources, DataFrame round trip, four UC health element, CGT projection and CGT response default files gave 377 passed on core 3.33.2 and on core 3.33.12.5dd27677, before any merge withmain(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 comes to 8,634. Memberships matched the dataset's 64-bit identifiers exactly, the 1,260 supplied variable-periods matched those before, the baseline became a new copy with the same records while the old baseline still held every household, every year ofdatasetheld the same records, and each year's household weight total stayed the same to 3 parts in 100 million. The same held forquantize_weights=False(9,864 households, 4.0 seconds).5dd27677: 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%).Aggregate impact
CI's aggregate impact report at
e804e8bc, merged intomain(040e9a5f07against4d3118a0ca,enhanced_frs_2024_25.h5, policyengine-uk-data 1.58.1, fiscal years 2025-26 to 2031-32), found:The one check failing on both sides,
sign:vat(VAT never goes negative), concerns code this PR does not touch. A full microsimulation does not callsubsample, and the changes tosupplied_inputs.pyonly add helpers.Found on the way
Simulation(dataset=<DataFrame>)failed for households of more than one person. Load a DataFrame simulation input as the dataset it describes #2270 fixed it.person_household_idandperson_benunit_idused 32-bit floats. Store person_household_id and person_benunit_id as integers #2273 fixed it. This PR reads memberships from the populations and does not depend on it.supplied_inputandsupplied_input_periodsreport a value the model calculated and cached over an input as supplied. Count only supplied inputs that are still stored in their tier #2308, still open, builds onstored_supplied_valuefrom this PR.🤖 Generated with Claude Code