Conversation
Auto-carry-over took the latest-starting stored period of any kind and returned the default if it started after the requested period. Any later stored period therefore hid an earlier input: a later input (inputs for 2012 and 2014 gave the default for 2013), or a later period the simulation had already calculated (2014 calculated first made 2013 the default). And values the simulation calculated carried like inputs: a twelfth cached at a month by calculate_divide (the policyengine-us monthly_age bug #557 fixes), a value masked by defined_for, a default cached where defined_for was false everywhere, or a formula result from before the formula's end. Results depended on which periods were calculated first. The rule now: a period takes the input stored for the latest-starting period that starts no later than it (on a tie, the one ending last), preferring the variable's own definition-period unit; with none, the default. Values the simulation calculates are stored with derived=True: the mark lives in the storage with the value (InMemoryStorage and OnDiskStorage keep it per key, every put sets or clears it, it counts only while the key is stored, and clones copy it), so deletions, direct writes and #556's shared arrays cannot leave it stale. Holder.is_derived(period, branch_name) reports the mark of the value get_array reads, so a value one branch calculated never hides another branch's input. A derived value never replaces an input the branch reads (calculate_add/calculate_divide used to overwrite inputs stored in another unit), calculate_add caches only sums over several sub-periods, and dump/restore keeps the marks. Storages gain has(), which neither reads nor copies an array. Supersedes #557's carry-over change: its 253 tests pass here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ut-only carry-over Round-2 review findings: - A default cached for a period with no earlier input became the base the uprating path uprated later periods from; the default is not cached for variables with uprating (as before). - A value calculated inside a set_input helper was written to the input's branch and recorded as a user input, bypassing the guard that keeps inputs; derived writes now stay on the branch they were calculated on. - dump_simulation reads each value's mark from the same branch and period as the value it dumps, so #552's branch dumps keep matching marks. - Carry-over checks candidates latest first and stops at the first input, instead of resolving the storing branch for every known period. - Storages drop the marks of deleted keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
force-pushed
the
fix-uprating-order
branch
from
October 2, 2026 06:57
5ddd21e to
1092dc6
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With no input at or before the period but a later period stored, master returned the default without caching it; caching it changed what formulas that test whether a value is stored see (policyengine-uk's maintenance loan and current_education formulas check get_array(period.last_year)), and gave the uprating path a calculated base. Return it uncached there, as master did; cache it (derived) otherwise, as master cached the value it carried. This replaces the uprating-only special case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
force-pushed
the
fix-uprating-order
branch
from
October 2, 2026 07:07
1092dc6 to
1e88ee1
Compare
MaxGhenis
force-pushed
the
fix-uprating-order
branch
from
October 2, 2026 07:11
1e88ee1 to
386cbf8
Compare
…he caches Round-3 review findings: - A period stored only under a branch this one cannot read was taken for an input (own-unit preference ranked it first) and read back as NaN. Holder.get_input_periods(branch_name) now lists, in one pass over the stored keys, the periods whose readable value is an input, taking for each period the key get_array reads first; carry-over picks from those. This also replaces the per-period walk up the branch chain. - apply_reform's cache wipe kept the storages' marks, and rebuilding a disk index brought a stale mark back onto a new input; the wipe now clears the marks and OnDiskStorage.restore starts without any. - Storages pickled before the marks existed failed to clone, put or delete; __setstate__ now defaults them (and #556's shared-key set). - The public carry-over rule now states the own-unit preference and that an input for the period itself is read back as stored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
force-pushed
the
fix-uprating-order
branch
from
October 2, 2026 08:01
386cbf8 to
a278233
Compare
…ebuilds Closes three mutants the suite did not kill. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two inputs covering the same days in units other than the variable's (month:2013-01:2 and day:2013-01-01:59) resolved to whichever was stored first. Prefer the larger unit, then the period's string form, so the choice depends only on the inputs (round-4 review). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
force-pushed
the
fix-uprating-order
branch
from
October 2, 2026 08:42
a278233 to
2120f78
Compare
This was referenced Oct 2, 2026
Simulation._calculate uprated a variable from its latest earlier stored period in its own unit, including periods the simulation had calculated itself, so the result depended on calculation order. Executed on fix-carry-over-order (index +3.7%/yr, input at 2012, 2015 asked alone vs after 2013 and 2014): - int: [1116, 85] alone, [1115, 83] after (truncation compounds) - float: float32 rounding compounds - defined_for false for one person in 2013 only: [1115.16, 1115.16] alone, [0.0, 1115.16] after (the 2013 mask carries into 2015) Also: a cached default, a monthly input carried into a calculated year, and a reform formula's result before its end each became an uprating source. For variables with uprating, keep only the earlier same-unit periods in Holder.get_input_periods(branch_name) (the carry-over PR's branch-aware provenance): periods whose value this branch reads is an input. That also drops periods stored only under a branch this one cannot read, which read back as None and raised TypeError. With no earlier input, the carry-over path or the default applies, as when the period is asked alone. Tests: tests/core/test_uprating_order.py (regressions, no hypothesis), tests/core/test_uprating_order_property.py (derandomized Hypothesis: any requests, branches and branch inputs, carry-over on/off == alone, byte for byte, and == reference rule), shared fixtures in tests/fixtures/uprating_order.py. #551's path-independence test is now exact instead of rel=1e-5. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
force-pushed
the
fix-uprating-order
branch
from
October 3, 2026 03:01
2120f78 to
57c0d36
Compare
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.
Stacked on #562 (carry-over order). This PR's change is the top commit only. Merge it after #562, or retarget once #562 lands.
Problem
Simulation._calculateuprates a variable that hasupratingand no formula result for a period. It takes the latest earlier period stored in the variable's own unit and scales that value by the index ratio. That period could be one the simulation had calculated itself: an earlier uprated, masked or truncated value. So an uprated value depended on which periods had been calculated first.Executed on #562's head (index +3.7% a year, input for 2012; 2015 asked alone vs after 2013 and 2014):
[1001, 77][1116, 85][1115, 83]: truncation compounds[1001.3, 77.7]1116.60729980468751116.607177734375: float32 rounding compoundsdefined_forFalse for person 0 in 2013 only[1115.16, 1115.16][0.0, 1115.16]: the 2013 mask carries into 2015The same drift happens when one of these, cached at an intermediate period, becomes the value uprated:
end.#551's path-independence test held only to
rel=1e-5, which hid the bitwise drift.Fix
Uprate only from an input this branch reads. #562 marks every value the simulation caches as derived.
Holder.get_input_periods(branch_name), also from #562, lists the periods whose valueget_array(period, branch_name)actually reads is an input. It ranks stored keys inget_array's lookup order (the branch, then its ancestors, thendefault; memory before disk) and leaves out periods stored only under branches this one cannot read. For variables withuprating, the uprating path keeps only the earlier same-unit periods in that set. With no earlier input, control falls through to #562's carry-over or the default, exactly as when the period is asked alone.It also fixes a crash that was already on master: a period stored only under a branch this simulation cannot read, for example through
Holder.set_input(..., "sibling"), used to be picked as the uprating source and read back asNone, which raisedTypeError. A regression test covers it.The production diff is one filtered list in
simulation.py, plus a comment in #562's no-input branch. Theknown_periods = holder.get_known_periods()line is unchanged, so #552 (branch-visible known periods) still rebases cleanly.Invariants
The domain: uprated variables of each value type (float, int, and bool masks via
defined_for), inputs in each variable's own unit (or any unit for a variable with noset_inputhelper), and earlier requests that are plain calculations in either unit,ADDorDIVIDE. Over that domain:-0.0differs from0.0). What a simulation returns for a period equals what a fresh simulation with the same inputs returns for that period alone. This holds when reading from the simulation, from a branch, or from a nested branch forked after those requests, including a branch that sets its own input. It holds with auto-carry-over on and off.index(period) / index(input period), cast to the variable's type and masked by its owndefined_for. With no such input, it is Carry over only inputs, the latest at or before the requested period #562's carry-over input or the default.Tests
tests/core/test_uprating_order.py: 33 regressions with no Hypothesis dependency, so the smoke job runs them too. They cover int truncation, float32 and monthly rounding,defined_formasks and all-false defaults, cached non-zero defaults, cross-unit carried inputs, a reform formula withend, branches and nested branches, branch-only inputs, and over-correction guards.tests/core/test_uprating_order_property.py: a derandomized Hypothesis property with 500 examples plus 9@examplerows. It covers invariants 1 and 2 byte for byte. Half the requests target the variable checked, and branches may set their own input. The docstring states the domain.tests/fixtures/uprating_order.py: the variables, the reference rule (which rejects inputs it doesn't model) andassert_bitwise_equal. It reuses Carry over only inputs, the latest at or before the requested period #562's simulation helpers.test_result_does_not_depend_on_intermediate_years_computed: now asserts exact equality instead ofrel=1e-5.b78b0ba935e20d9f)57c0d36b)The cases that pass on the bases are the over-correction guards.
Mutation check (
mutants.py): all 6 mutants of the filter are killed, and the property alone kills all 6. The mutants are: no filter, a filter that ignores the branch, an inverted filter,maxover the unfiltered list, dropping theupratingguard, and uprating from the earliest input.Local runs on this head: full suite 1234 passed, 4 skipped, 1 xfailed. Country-template YAML: 39 passed.
ruff format --check: clean.Independent reviews. A final-commit review (Opus, standard tier, on
2120f78f) returned APPROVE WITH NITS. It found no production defect and passed its own 400-example holder-level differential and 300-example tree property; its nits about stale text are fixed here. An earlier review (GPT-6.1 Sol, hard tier, on a previous head; scripts and report kept with the evidence) also returned APPROVE WITH NITS, with no production defect introduced by this change. That earlier review's test nits are fixed here:Measurements
These are real PE-UK and PE-US microsimulations, with no scaling. Cores compared: master
b78b0ba9, #562 and this PR. The full-eCPS and multi-year runs used #56227d1bc35with this change at1e88ee16. A confirmation pass on #56235e20d9fand this PRa2782333reran PE-UK single and multi, the PE-US subsample and full-eCPS 2026. Every comparison matched the earlier pass. The final heads (#562912a4ac5, this PR2120f78f, whose production code57c0d36bkeeps) changed only #562's carry-over tie-break for equal-extent inputs in another unit. A check of the final heads against the confirmation outputs matched byte for byte: UK 2031-2033 41/41 arrays each year, full-eCPS 2026 30/30, and the 2025-2026 subsample 30/30 each year, for both #562 and this PR. Every run on this PR was repeated with a shadow logger, which left outputs bitwise unchanged; it records each uprating decision where master's rule picks a different source period.core-cow-bench) and the full sample.Single-year simulations (what policyengine.py and the APIs build): no output changes.
spm_unit_net_incomefor 14 SPM units: #562's state_fips/FPG fix, see #562.)Multi-year in one simulation: this PR removes uprated-input drift.
The multi-year differences that remain are not uprating:
current_educationformula copies last year's value only if last year happens to be cached, and otherwise imputes from age. That's a PE-UK order dependence. Separate task.ctc_limiting_tax_liabilityandtax_liability_if_itemizingfor 63-120 units). These are computed in PE-US's persistentitemizing/not_itemizingbranches, created in 2025 and reused in 2026. policyengine-us#9738 (get_branch_for_period) re-forks them.ParameterNotFoundError(md...informal.rates.UNKNOWN) on Carry over only inputs, the latest at or before the requested period #562 and this PR. It doesn't raise on master, but master's 2026 is wrong there (income tax $33bn, the Carry over only values stored at the variable's definition period #557 age/12 bug). The cause is in PE-US: itscountyformula reads the default branch's latest period from inside a reused branch. The carry-over session is filing the PE-US fix.spm_unit_is_in_spm_povertyraisesSPMInputErroron every core, master included. This is the known eCPS SPM composition defect, us: name the SPM composition defect in seconds, and refuse it by name in the release microcosm#948. It isn't measured here.Not in scope
These are pre-existing on master; the review found most of them.
set_input: Make set_input on a branch drop values calculated from the input it replaces #560 covers branches; nothing is invalidated for the root simulation, as on master.restore_simulationdropping the input registry, a staleHolder.set_inputfast cache, and Enum uprating raising: new task.axiom: n/a: engine fix to uprating source selection in policyengine-core; no policy rule changes
🤖 Generated with Claude Code