Repository navigation
Count only inputs as already set when a set_input helper splits a longer period - #581
Merged
Merged
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>
MaxGhenis
marked this pull request as ready for review
October 3, 2026 18:31
MaxGhenis
added a commit
that referenced
this pull request
Oct 6, 2026
set_input for one of a variable's own periods (a month of a monthly variable, a year of a yearly one) replaced the stored value but left what calculate had derived from it at periods of another size: the sum of a monthly flow over a year, a yearly flow's twelfth at a month. A later calculate returned the old result, so it depended on whether that period was read before the input was set (#579). Warm annual flow 0, January set to 1: the annual read stayed 0. Holder.set_input now applies the rule the set_input helpers follow (_drop_calculated_overlapping): after storing an input for one of the variable's own periods, it drops the values the simulation calculated for the variable at overlapping periods, on the input's branch and the branches it reads, and evicts calculate's fast-cache entries for overlapping periods. Inputs are kept. ETERNITY variables are left alone. Stacked on #581, whose helper it reuses. Removes #581's property exclusion for #579, so test_inputs_do_not_depend_on_what_was_calculated_before now compares reads of other-size periods for every variable. On #581's head that property fails, shrunk to an annual flow_m read on a branch of depth 2; with this change it passes. Adds seven regression examples, all of which fail on #581's head. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
#593 is stacked on this branch to fix #579, the own-period path this PR's docstring leaves out. #593 touches only |
This was referenced Oct 7, 2026
Preserve supplied inputs per storage tier when helpers split periods, register direct helper writes through Holder, and discard overlapping non-input caches. Retain master dumper and option-cache contracts; cover provenance, replay, no-op helpers, and early years with focused regressions.
Contributor
Author
|
US + core hub merge audit, core#581 at
|
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
1 participant
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.
Period-splitting input helpers now count only surviving supplied inputs as already set. They use the same per-tier record as reform replay, so a memory cache cannot impersonate a supplied input or hide a surviving disk input. Direct helper calls register normalized input periods and storage tiers. Successful helpers remove overlapping calculated values and cache shadows, including when every sub-period already has an input and when storage keys contain early years.
The conflict resolution retains master's storage provenance, supplied-input tier record, option-result cache contract, and simulation dumper. The duplicate dumper implementation has been removed in favor of #576's implementation. Helper docstrings describe the behavior; separate documentation changes are not required because public signatures and development workflows are preserved.
Merge commit
e598a2d9e57b9d7652ac21874e27be81978f3c56brings master ata3618dbc983541a4fe5732ee6d9a977aad122794into this branch. The branch was not rebased.The d899 prerequisite hold is satisfied: #576 (squash commit
7ad23dc4, PR headb8d98660a0), #558 (f929fe5e), #561 (0d1d154e5a), and #571 (13e58064e1) have all landed on master. All four landed commits are ancestors of this merge. Current-head CI and the hub Core A/B gates remain required.Adds 20 focused regression cases for unregistered carry-over caches, replaced input tiers, disk inputs hidden by memory caches, successful no-op helpers, and early-year overlap invalidation. Direct-helper registration coverage also expands to both memory and disk. The tier interaction regressions compare reads and exported inputs against fresh simulations before and after reform replay.
Validation uses
uv run pytest -q -p no:cacheprovideron Python 3.13.15 with Hypothesis installed, with focused files run individually in the foreground, followed by onetests/corerun:tests/core, onceAll 12 selected tier regressions fail with the original #581 helper on the composed master; the resolved helper passes all 60 tests in the example file. Ruff lint and formatting checks pass on the changed Python files.
Independent source review approved the integration: impact medium, confidence high. Focused and Core validation passed. No microsimulations were run, and the PR has not been merged. The hub's Core A/B is under Impact.
Impact
What changes in model output. Results change only when a value has been calculated or cached for a sub-period before a period-splitting
set_input. Example: a flow variable's month is read, then its year is set to 1,200. On master the cached default stays in that month and the other 11 months get 1,200/11 each. With this PR every month gets 100. Normal dataset loads set inputs before any calculation, so population runs shouldn't move.Hub A/B microsimulation, PE-US, current head. The hub ran PolicyEngine-US main
fd681b9ad0on the default dataset with Core mastera3618dbc98(the merge base) and with this PR's heade598a2d9e5, for 2025 and 2026. Every compared array is identical:household_net_income,income_tax,state_income_tax,household_benefits, and the household and person-household IDs. Run time and footprint are unchanged. In 2026 the base took 167s and this PR 177s; the base's 2025 time includes lock wait, and this PR took 172s. Both cores peaked at 11,264 MB (2025) and 12,288 MB (2026). All 19 CI checks pass at this head.