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>
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>
supplied_input and supplied_input_periods checked only that core recorded the input and that Holder._get_array_from_storage returned something. That reads memory before disk and does not ask whether the value is the input, so a value cached over a supplied input counted as supplied, and an input on disk under a value cached in memory was read from memory. Both now use the tier rule of stored_supplied_value (Holder's _stores_user_input), keep the nearest-branch-first order, and keep the neutralised-variable behaviour. Fixes #2307. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Example tests for a value cached over an input in memory and on disk, an input on disk under a value cached in memory, branch precedence and a neutralised variable. A Hypothesis model of inputs set, values cached, arrays deleted and calculations run over a three-branch chain and both tiers checks the helpers against the model, against each other, and against policyengine-core's Holder._stores_user_input and (from 3.33.1) Simulation._get_recorded_input_array. A last test calculates with every registered variable supplied and checks each input is still supplied, so normal calculation never hit the defect. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On policyengine-core 3.33.2 and later, the property test also checks that the periods the helper lists are the periods core lists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
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>
juaristi22
left a comment
There was a problem hiding this comment.
Reviewed at 5b4dc2143347b49226d06fc9e6421495da4e5cfe, using the stacked merge-base diff over #2265. No actionable findings in surviving-input storage tiers or affected consumers.
Both helpers use the same eligibility predicate as Core’s input-store checks, including recorded-empty locations, derived values, memory/disk tier order and overwritten child-branch inputs. The unchanged subsampling API retains its behavior.
Validation: all 104 tests in the five affected files passed, including the three-branch/two-tier properties, ordinary household net income and income tax, bus fare and NI loss consumers, and subsample rebuilds. Ruff formatting passed. Execution used Core3.33.2; I also compared the identical predicate in the pinned Core3.32.27 source. The documented #2265 merge dependency remains.
|
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
Resolve the conflicts in policyengine_uk/simulation.py (keep both sides' imports) and policyengine_uk/utils/supplied_inputs.py: keep main's model-value records (set_model_value) and population copying, and this branch's tier rule. supplied_input reads through stored_supplied_value and then leaves out a model value; supplied_input_periods finds the tier without reading the value, and reads it only to tell a model value apart. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6FT2A5BnVxvVkQuCTo
policyengine-core 3.33.0, which main now requires, checks a single tier with _stores_user_input(period, branch_name, storage_name). Use it instead of this module's copy of the same rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6FT2A5BnVxvVkQuCTo
Main registers 45 variables in SUPPLIED_INPUT_VARIABLES, among them booleans, enums, integers and monthly variables. Store a value of each variable's own type for its own period, rather than a float for a year. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6FT2A5BnVxvVkQuCTo
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LwQq6FT2A5BnVxvVkQuCTo
|
Generated by Claude Code |
|
Generated by Claude Code |
|
Ready for hub review Head: Commits on top of
Open findingsNone. The PR has one review, Conflict resolutions
Tests changed by the merge
Commit Tests run (all at
|
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
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; standard reform costings (change in gov_balance); standard reforms: change in each reform's target. Checks failing on both base and head (1)
Counted-once checkEach of 92 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
|
Fixes #2307
Reproducer: policyengine_uk/tests/test_supplied_inputs.py::test_a_value_cached_over_an_input_is_not_supplied
axiom: n/a: simulation infrastructure, no policy rule change
Stacked on #2265. #2265 adds
stored_supplied_value, and this PR builds on it rather than adding a second copy. This branch contains #2265's head,e804e8bc, and targetsmain, so CI runs on it. Land #2265 first: this PR's diff then shows only its own three files.This branch merges
mainat 47a2817 (merge commit 3bce60e).mainhad added model values (set_model_value), copied populations and 38 more variables inSUPPLIED_INPUT_VARIABLES, and requires policyengine-core 3.33.0 or later (uv.lockpins 3.34.1).0e44f0988mergese804e8bcinto71fb10ca, with two conflicts. Insimulation.pyboth sides added thesubsample_simulationimport, and the merge keeps one copy. Inutils/supplied_inputs.pythe module docstring keeps this PR's paragraph on tiers, which covers every helper, and #2265's sentence on model values insupplied_input_arrays. The merge also keeps this PR's_supplied_without_readingand #2265'sis_model_value.What was wrong
supplied_inputandsupplied_input_periodstreated a value cached over a supplied input as supplied. #2307 reproduces it: afterset_inputof 100 andput_in_cacheof 999 for the same period,supplied_inputreturned[999.]andsupplied_input_periodsreturned[2025], while core'sHolder._stores_user_inputreturnedFalse. The same script onmainat 47a2817 with policyengine-core 3.34.1 still prints[999.]and[2025], and core'sSimulation._get_recorded_input_arrayreturnsNone.Why
Both helpers checked only two things: that core recorded the input (
Simulation._user_input_keys), and thatHolder._get_array_from_storagereturned something. That method reads memory first, then disk, and does not ask whether the value it finds is the input. The record outlives a value cached over the input.put_in_cachestores over the input and takes that tier off the holder's record of where the input is (Holder._user_input_storage), but the_user_input_keysentry stays. So there were two wrong answers:What changes
supplied_inputreads throughstored_supplied_value(from Give UK simulations a working subsample #2265). That reads a supplied value only from a tier in which core'sHolder._stores_user_input(period, branch_name, tier)finds the input: a tier that the holder records the input in and that holds a value the engine did not calculate. Memory comes before disk.supplied_input_periodsuses the same rule. It finds the tier without reading the value, which may be on disk. It reads the value only when the model stored one for the period (set_model_value), to tell the model's value from a supplied one, assupplied_inputdoes.stored_supplied_valueand the periods helper share a private_supplied_value_storage. Whatstored_supplied_valuereturns stays the same.supplied_inputstill looks at this branch first, then each branch it was made from, then the default branch. If a branch's input has gone, it reads the next one, as core's_get_recorded_input_arraydoes.set_model_valuewhile it stays the stored value.SUPPLIED_INPUT_VARIABLESand its check.drop_missing_supplied_inputs. It forgets a record whose value has gone from both tiers, the population-rebuild case (policyengine-core#605). It keeps a record whose input a cached value has replaced. That does no harm now: the helpers ignore such a record, and so does core's input export (_stores_user_input).Does this change results?
Not in normal calculation. The defect shows only when code stores a value with
Holder.put_in_cacheand its defaultderived=False, or withHolder._set.Simulationcallsput_in_cacheonce, for each value it calculates, withderived=True.put_in_cachewithderived=Truestores nothing when the valueget_arraywould read for that period is an input.set_inputand its period helpers, and the input replay in_invalidate_all_caches.put_in_cachenorHolder._set.put_in_cachecalls only in US code: one crfb-tob-impacts worker and two policyengine-us tests.test_calculating_leaves_every_supplied_value_suppliedexecutes this claim. It supplies all 45 variables inSUPPLIED_INPUT_VARIABLESfor two years, each with a value of its own type for its own period, on the default branch, and one more input on a branch. Then it calculates ten outputs over five years on both, and checks that each input still counts as supplied and keeps its value, and that core still counts every recorded input as stored. This test also passes on the code before the fix.The callers that read through the helpers, such as the bus fare, NI class 4 loss, carer benefit, childcare, Housing Benefit, PIP and energy price guarantee formulas, get a different answer only after one of the routes above has cached a value over the input.
Invariants
policyengine_uk/tests/test_supplied_inputs.pystates them. The model covers every sequence of these steps: inputs set, values cached (calculated or not), arrays deleted and calculations run. Each step can act on a branch, on the branch it was made from or on the default branch, in memory or on disk.supplied_inputreturns the input on the nearest branch that still stores one for the period, reading memory before disk. Otherwise it returnsNone. An input survives until something deletes it, or until another input or a cached value replaces it in the same tier. A value cached in memory over an input on disk leaves that input. A calculated value never replaces an input.supplied_inputreturns a value for a period exactly whensupplied_input_periodslists that period, and the list runs from the earliest period.Holder._stores_user_inputdoes.supplied_inputreturns whatSimulation._get_recorded_input_arrayreturns.supplied_input_periodslists the periods thatSimulation._get_set_input_periodslists.Intended: a neutralised variable whose input survives reads as its default.
Tests
policyengine_uk/tests/test_supplied_inputs.pyhas 53 tests:set_inputfirst drops cached values that may depend on the input, so an input can go to disk where the model expected memory. The model now records the tier that holds each value, and an explicit example pins the case.uv sync --frozen --extra dev),-n 4and a fresh--basetemp: this PR's test files and every test file that imports the helpers gave 590 passed. The 16 files:test_supplied_inputs.py,test_subsample.py,test_bus_fare_reforms.py,test_input_records.py,test_ni_class_4_profits_properties.py,test_gift_aid.py,test_housing_benefit_accommodation_dates.py,test_input_dataframe_export.py,test_capital_gains_projection.py,test_household_subset.py,aggregate_impact/test_aggregate_impact_childcare_provenance.py,aggregate_impact/test_aggregate_impact_guard.py,test_trading_allowance_claim_status_data_year.py,test_childcare_birthday_terms.py,test_benefit_cap_specified_benefits_properties.pyandtest_current_education_order.py.policyengine_uk/tests/policythat name a variable inSUPPLIED_INPUT_VARIABLESgave 3,420 passed. They includebus_journeys,person_bus_fare_spending,loss_relief,ni_class_2andni_class_4_profits(68 passed on their own).policyengine_uk/tests/code_healthat 71fb10c: 3,163 passed.test_supplied_inputs.py:Holder._get_array_from_storage, withmain's model-value check kept): 51 of 53 fail. The two that pass,test_a_calculated_value_is_never_stored_over_an_inputandtest_calculating_leaves_every_supplied_value_supplied, should pass before the fix too.test_input_records.py, run alongside, catches none. A default single-person simulation stores model values ofuc_LCWRA_elementfrom 2026, and the mutant lists those years as supplied.uvx ruff@0.17.2 format --check .,uvx ruff@0.17.2 check .,uvx ruff@0.17.2 check --select F811,F821 policyengine_ukandgit diff --checkpass at 71fb10c.0e44f0988. CI runs the suite there.0e44f0988, merged intomain(040e9a5f07against6fe91ff6e8,enhanced_frs_2024_25.h5, policyengine-uk-data 1.58.1, fiscal years 2025-26 to 2031-32), breaks no check and flags no moves, and no household's net income changes in any year. One check,sign:vat, fails on both sides, as it does onmain.🤖 Generated with Claude Code