Conversation
…ger period set_input_divide_by_period and set_input_dispatch_by_period treated any value stored for a sub-period as already set, including values the simulation had calculated (a cached default or formula result). The same annual input then gave different months depending on what was calculated first: a divided input skipped calculated months and shared the rest among the others, or failed as inconsistent after the year had been read; a dispatched input reused a calculated month for every later month. The helpers now read the simulation's record of inputs (_user_input_keys): a recorded sub-period keeps its input (and, for dispatch, passes it on to the later sub-periods, as before), and a calculated one is replaced. After storing, they drop the variable's calculated values over overlapping periods (the sums calculate_add caches, the twelfths calculate_divide caches) under the input's branch and the branches it reads through, record what they store as inputs, and evict calculate's fast cache for those periods. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
calculate does not put sums or twelfths in its fast cache today, so this is defensive: a value held there for a period whose stored value the helper drops goes with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A dump does not record which values were inputs, and restore_simulation stored every value with put_in_cache, so a restored simulation had an empty input record and the helpers replaced restored inputs (review finding 1). Restore now stores each value as a recorded input, the rule #576 applies to dumps without an inputs.txt. Tests: a restored month keeps its value under a yearly input (divide and dispatch); an input on a nested branch drops the sum its parent branch calculated; an input stored for twelve months from March is kept. The order-independence property now creates up to two nested branches, with calculations at each level. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On disk, a value calculated for a period like year:2013:2 is stored in a file named after the period, and Windows rejects ":" in file names (policyengine-core#526). The Windows CI jobs failed on such a read in the on-disk examples. In disk mode the property now skips those periods. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… replaces Review round 2: 1. Restoring every dumped value as an input froze calculated values through a later reform. The dumper now writes, next to each variable's arrays, the periods whose value was an input (inputs.txt), and restore records exactly those; a dump without the file restores every value as an input. This is policyengine-core#576's dumper change, taken byte for byte so the two PRs merge without conflict. 2. A value stored by put_in_cache over an input (say a calculate_add sum over an input set for twelve months) left the input's record entry in place, so the helpers and apply_reform kept treating the calculated value as an input. Holder._set now drops the entry, in both of its forms for twelve months from the first of a month, when it stores a value outside set_input. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…culate_add With policyengine-core#571, calculate_add no longer caches a sum over an input a plain read finds, so the input is kept and the test's premise (master's calculate_add overwriting it) did not hold. Storing the calculated value with put_in_cache replaces the input with or without #571. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ly for read branches Review round 3: - Holder._set no longer drops a record entry when a calculated value is stored over an input. A calculate_add at a variable's own period stores the unchanged input and lost its record; a clone, which shares its source's record until #561, deleted the source's entries; and ETERNITY variables keep entries under several periods. The case the cleanup was for, calculate_add caching a sum over an input, is what #571 stops. - Master now gives a storage that shares nothing a frozenset for _shared (#578). The drop step deleted from it directly; it now leaves that to InMemoryStorage._stop_sharing_dropped_keys. - The helpers evicted calculate's fast cache for a sub-period stored under any branch name, including one the simulation does not read. They now evict only for branches it reads, as #576's holder-write property requires. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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 #580.
Summary
set_input_divide_by_periodandset_input_dispatch_by_periodspread an input given for a longer period over a variable's months (or years). They treated any value stored for a sub-period as already set, including values the simulation had calculated (a cached default, a formula result, a carried-over or uprated value). So the same input gave different values depending on what had been calculated before it was set.Executed on master b78b0ba, with
CountryTaxBenefitSystemplus formula-less person variables and one person:flow_m: float, MONTH (divide)calculate("2013-01"), thenset_input("2013", [1200]); read May and Januaryflow_mcalculate("2013"), thenset_input("2013", [1200])ValueError: Inconsistent inputstock_m: int, MONTH (dispatch)calculate("2013-01"), thenset_input("2013", [7]); read Maystock_mcalculate("2013"), thenset_input("2013", [7]); read December and the yearflow_y: float, YEAR (divide)calculate("2013"), thenset_input("month:2013-01:24", [2400]); read 2013 and 2014flow_ycalculate("2013-05"), thenset_input("month:2013-01:24", [2400]); read 2013-05(
repro/helper_order.pyin the evidence folder; with this branch every "after calculating first" cell equals the "new simulation" cell.)The helpers now tell inputs from calculated values using the simulation's record of inputs (
Simulation._user_input_keys). That record has to be right for them to be right. Where master gets it wrong, this PR either fixes it or names the open PR that does (see "Merge order" below).restore_simulation. It stored every restored value withput_in_cache, so a restored simulation had an empty record and the helpers would have replaced restored inputs. The dumper now writes which values were inputs (inputs.txt), and restore records exactly those; a dump without the file restores every value as an input. This is #576'ssimulation_dumper.pychange, byte for byte: the two PRs merge without conflict on that file, and whichever lands second changes nothing there.The rule
A sub-period counts as already set only if its value was stored as an input, meaning it is in the record. Entries are
(variable, branch, period); an input set for twelve months from the first of a month is stored under the year, so both forms are recognised.A recorded input is kept, as before:
A calculated value is replaced, as if nothing were stored.
Calculated values over overlapping periods are dropped after storing. These are the same variable's values over periods that overlap the input period:
calculate_addcaches (a monthly variable's year);calculate_dividecaches (a yearly variable's month);The search covers the input's branch name and the names its simulation reads through (its ancestors, then
default), in that simulation's own memory and disk storage. Inputs are never dropped, and the simulation a branch was created from keeps its values.Bookkeeping. What the helpers store is recorded even when a helper is called directly.
calculate's fast cache drops the replaced and dropped periods, but only for a branch the simulation reads: a value stored under a branch it does not read changes nothing it returned.No record. A simulation without one (
_user_input_keysmissing), or a holder without a simulation, keeps master's behaviour: every stored value counts as set.The dispatch helper's "reuse the existing array" branch (the TODO)
This branch is kept for inputs, and the docstring now documents it. A sub-period that already holds an input keeps it, and that input, not the value given for the longer period, is applied to the later sub-periods that have none. With
3set for March, setting7for the year gives7in January and February and3from March to December: a stock's value known at a month holds for the rest of the period.That is the existing behaviour for inputs set on top of inputs, which the issue requires to keep working. Changing it would change results for sequences with no calculation at all, a separate semantic question. The asymmetry stays: the reverse order gives
7in every month but March. A calculated value is no longer reused this way; that was the bug, where one calculated January set every later month to the default.Invariants
Each holds for any variables, inputs and calculation requests in the property tests' scope (
tests/core/test_set_input_helper_order_property.py):calculaterequests (on the simulation and on up to two nested branches, a list per level), then inputs I2 on the last of these. The result matches setting I1 and I2 on a simulation that calculated nothing:Referenceintests/fixtures/set_input_helper_order.py), with the two helper rules above applied to inputs.apply_reformrecalculates restored calculated values.Outside the properties, because they are other order dependences with their own PRs or issues:
calculate_add/calculate_dividecalled directly (Cache ADD and DIVIDE results only where a plain read returns them #571);Tests
tests/core/test_set_input_helper_order.py, 39 example tests. On master cbfdedf, 26 fail and 13 pass. The 13 that pass pin behaviour that must not change:The restore and fast-cache tests pass on master and fail on this branch's helpers without the matching change.
tests/core/test_set_input_helper_order_property.py, invariants 1-4, Hypothesis with 500, 300 and 300 examples. The module starts withpytest.importorskip("hypothesis")for the smoke job. All three fail on master within the first examples. Soak runs passed: 8,000/4,000/4,000 examples at af877e8 and 6,000/3,000/3,000 at 1f5e3ec. The helper logic they exercise has not changed since, apart from the fast-cache condition, which the example test above covers.Mutation check (
mutation/run.py): 25/25 mutants killed at 917f0c2. They include the two mutants the review found surviving at 22a9b31 (twelve-month inputs recognised only from January; intermediate ancestor branches skipped), the restore change, and the fast-cache condition.Full suite at 917f0c2 (merged with master cbfdedf): 1,208 passed, 4 skipped, 1 xfailed. Country-template YAML tests: 39 passed.
Windows: on disk, a value calculated for a period like
year:2013:2goes in a file named after the period, and Windows rejects:in file names (Storage keys need a structured (branch, period) scheme: str(period) is lossy and separators collide with branch names #526). The property's on-disk examples skip such periods. With:blocked in file names the way Windows does it, the property fails at 1f5e3ec and passes at f7ac58b.Review
Independent adversarial review, GPT-6.1 Sol, four rounds (
review/in the evidence folder):holder.pyis master's again. That case now depends on Keep the record of set_input values in step with storage #561 and Cache ADD and DIVIDE results only where a plain read returns them #571 together (next section). The same round's composition with Restore inputs as inputs, evict the fast cache on holder writes, reject non-numeric uprating and defined_for #576 found the helpers evicting the fast cache for branches the simulation does not read; fixed.Merge order: #561 first; #558 and #571 before or with this
The cases below come from the review, executed on master, on this branch at 917f0c2, and on this branch merged with #561, #571 or #558 (
review/review_lifecycle.py,review/round2_scripts/replacement_probe.py; outputsreview/lifecycle_917f0c23.out,compose/round4/replacement_561_571.out):set_inputhandler calculates while storingdelete_arrays, recalculated, then an annual inputInconsistent inputdelete_arrays, written during handlers. Keep the record of set_input values in step with storage #561 fixes that. In rows 1-3 this branch is exactly as wrong as master. In row 4 master raises and this branch returns a stale year. So Keep the record of set_input values in step with storage #561 should merge first.calculate_addover an input (round 2, finding 2). Callingcalculate_adddirectly over an input set for twelve months, then setting an annual input, is the same kind of case. This branch alone, and with Keep the record of set_input values in step with storage #561 alone or Cache ADD and DIVIDE results only where a plain read returns them #571 alone, gives a stale annual[0, 0]. With Keep the record of set_input values in step with storage #561 and Cache ADD and DIVIDE results only where a plain read returns them #571 together it matches a simulation that never calledcalculate_add: the annual read keeps the twelve-month input, the months take the annual one, and both survive invalidation. Cache ADD and DIVIDE results only where a plain read returns them #571 stops ADD caching over an input, but it looks the input up by the year, and only Keep the record of set_input values in step with storage #561 records a twelve-month input in that form.clone.set_input(...)already does this on master, as does a default-branch holder input on aget_branchbranch (executed,repro/disk_clone_exact_input.py). This branch adds one more such write. With Give each disk-backed holder storage its own directory #558 merged none of them reaches the source. Disk storage is opt-in (MemoryConfig, marked experimental).How it composes with the other open core PRs
Each PR head was scratch-merged with this branch at 917f0c2, and the full suite was run on the merge (
compose/round4/):_user_input_keysdrift)Restore inputs as inputs, evict the fast cache on holder writes, reject non-numeric uprating and defined_for #576. Its restore property excludes, from the comparison with a new simulation, the examples where an annual input follows a calculation, because of this bug. With the exclusion removed (
compare_with_fresh = True) at 3,000 examples, it fails on Restore inputs as inputs, evict the fast cache on holder writes, reject non-numeric uprating and defined_for #576 alone on exactly this case and passes merged with this branch. Once both are in,_split_after_calculatingcan go.Carry over only inputs, the latest at or before the requested period #562, Uprate only from inputs, so uprated values don't depend on calculation order #563, Make set_input on a branch drop values calculated from the input it replaces #560, Read only periods the current branch can see when uprating or carrying over #552. None of these conflicts is this PR's own:
simulation_dumper.py, they are Restore inputs as inputs, evict the fast cache on holder writes, reject non-numeric uprating and defined_for #576's conflicts (git merge-treeof each head with Restore inputs as inputs, evict the fast cache on holder writes, reject non-numeric uprating and defined_for #576 conflicts on the same file), since this branch's dumper is Restore inputs as inputs, evict the fast cache on holder writes, reject non-numeric uprating and defined_for #576's.in_memory_storage.py, because of Give a storage a set of shared keys only while it shares an array #578.At an earlier head, each was resolved and its full suite passed: Carry over only inputs, the latest at or before the requested period #562 (1,243) and Uprate only from inputs, so uprated values don't depend on calculation order #563 (1,277) write and read both sidecars, Make set_input on a branch drop values calculated from the input it replaces #560 (1,269) takes its own dumper (it already records inputs), and Read only periods the current branch can see when uprating or carrying over #552 (1,718) dumps the branch-visible values with the input record. The resolved files are in
compose/round3/.Carry over only inputs, the latest at or before the requested period #562. Adds a per-key
derivedmark in storage. After Carry over only inputs, the latest at or before the requested period #562, the helpers could read that mark for the exact branch they store to. Itsis_derivedreads through ancestors, which is the wrong question here: a parent's calculated value underdefaultmust not stop a branch from taking an input. The drop step deletes keys from_arrays/_filesdirectly; Carry over only inputs, the latest at or before the requested period #562'sis_derivedrequires the key to still be stored, so a stale_derivedentry is harmless.Make set_input on a branch drop values calculated from the input it replaces #560. Keeps its own
_input_keysand_sequence_numbersin storage. The drop step removes only non-input keys. A stale_sequence_numbersentry for a dropped key is ignored by every reader and cleared by the nextdelete.Downstream
A/B, real runs. Core master cbfdedf against this branch at 917f0c2, with the same country code and data, every array compared byte for byte:
The same comparison at 22a9b31 and 29410c3, against master b78b0ba (the PE-US run on another enhanced CPS 2024 file), was also bitwise identical.
On these runs the only code that differs from master is the helpers' body, which runs twice in PE-US and not at all in PE-UK. Timings were within run-to-run noise on a loaded host:
Code that sets inputs over longer periods. Every non-test
set_inputcall was listed, with the definition period of the variable it sets (downstream/set_input_sites.py; it makes no changes to the repos it reads):upstream/main615a0f2dorigin/main84da2863upstream/master389648adEvery call that names its variable passes a period of that variable's own unit, so no helper runs. The calls whose variable is a parameter fall into these groups:
simulation.pybuild_from_*, PE-USsystem.py, policyengine.pymain6a9c878us/model.py). These set inputs on freshly built populations before anything is calculated.move_valuesand dynamics, PE-USbehavioral_response_measurements,tob_revenue_*). All set YEAR variables at years.set_input = set_input_dispatch_by_periodattributes. All are on YEAR variables.utils/dependencies.pycalculate_dependency_contributions. This is the only site where a helper meets calculated values: it zeroes and then restores a dependency withset_input(var, year, ...). For a MONTH float dependency it fails either way: on master the zeroing call raisesInconsistent input, and with this branch the restoring call does (repro/uk_dependencies_pattern.py). Nothing relied on the old behaviour. Fixing the utility is a separate PE-UK task.No downstream code calls
dump_simulationorrestore_simulation.Checks
ruff formatandruff checkclean.Evidence (scripts, outputs, A/B arrays, mutation results, compositions, reviews):
~/reviews/core-set-input-helper-order-2026-10-02/on the author's machine.axiom: n/a: core engine input handling, no policy encoding
🤖 Generated with Claude Code