Repository navigation
Break equal-start uprating ties the way auto-carry-over does - #582
Merged
Merged
Conversation
MaxGhenis
force-pushed
the
fix-uprating-equal-start
branch
from
October 3, 2026 12:19
c668f1e to
f051cc2
Compare
MaxGhenis
added a commit
that referenced
this pull request
Oct 3, 2026
The carry-over sort key evaluated period.stop for every candidate input. A period that ends after 9999-12-31 (day:9999-12-30:3, or day:2012-01-01:4000000) has no stop: computing it raises OverflowError. Master carried such inputs ([10, 20]); this branch raised. Sort by _end_order, which puts such a period after every period that has a stop, so it still ends last. Found by the review of #582, which uses the same key for uprating. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
force-pushed
the
fix-uprating-equal-start
branch
2 times, most recently
from
October 3, 2026 18:35
e12993c to
2e00515
Compare
MaxGhenis
added a commit
that referenced
this pull request
Oct 3, 2026
…562) * Carry over only inputs, the latest at or before the requested period 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> * Keep uprating, input helpers, dumps and deletions consistent with input-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> * Test that deleting a whole branch drops its derived marks Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Cache the default only where master cached a carried value 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> * Read input periods in one pass over readable keys; clear marks with the 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> * Test memory-over-disk precedence, in-memory mark clearing and index rebuilds Closes three mutants the suite did not kill. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Break equal-extent carry-over ties by unit, then period 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> * Record a storage's inputs, not its derived values, and share them with clones Measured on a policyengine-us household (policyengine-us 2.18.3, one household_net_income calculation, 6,185 storages, 30,925 holder clones across 5 branches), master with #578 retains 20.94 MB per simulation. With derived marks as a set per storage that holds a derived value, the merge retained 24.35 MB: 1,514 sets in the simulation and a copy of its source's marks in every holder clone. That is 2.6 times the 1.3 MB per simulation that ran policyengine-us's CI out of memory (#577). 5,201 of the 5,204 stored values are derived, so record the inputs instead: is_derived is "stored and not an input". The 3 storages holding inputs have a set (648 bytes); the rest read _NO_INPUTS. A storage and its clones share one frozenset of inputs until one of them changes its own. Retained memory is now 21.25 MB per simulation, +0.31 MB on master: one more 8-byte attribute slot in each of the 37,110 storage objects. A pickle records that inputs were recorded; a state from before (no marker) counts every stored value as an input, as it did then. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Carry over an input that ends after the last representable date The carry-over sort key evaluated period.stop for every candidate input. A period that ends after 9999-12-31 (day:9999-12-30:3, or day:2012-01-01:4000000) has no stop: computing it raises OverflowError. Master carried such inputs ([10, 20]); this branch raised. Sort by _end_order, which puts such a period after every period that has a stop, so it still ends last. Found by the review of #582, which uses the same key for uprating. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Order period ends by day number, so ends after 9999-12-31 compare too 6ddbaf9 sorted every period whose stop cannot be computed after those that have one, but equal to each other, so of two inputs ending after 9999-12-31 the string form decided: day:9999-12-30:9 was carried over day:9999-12-30:10, and 24 months over 800 days from the same day (delta review, P2). Month and year periods past 9999 also raised ValueError, or returned a stop no date can hold, rather than OverflowError. _end_order is now the number of the day after the period's last day, counted as date.toordinal counts, by integer arithmetic on the same calendar. It equals stop + 1 day wherever stop has a value and orders every other period by its true end. Tests: the review's two cases in both set orders; a property that _end_order agrees with Period.stop where it has a value and with numpy's calendar everywhere (1,000 examples, sizes to 5,000,000); a longer period from the same day ends later. Also asserts that a pickled state without a set of shared keys gets none (a surviving mutant). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Two earlier inputs in a variable's own unit can start on the same day: Holder.set_input stores a year:2012:2 input to a yearly variable as given (a set_input helper is only called for another unit), beside one for 2012. The uprating source was max(..., key=start), so the tie went to whichever was stored first, or to the one in memory over the one on disk: 2015 came out as [1.115, 1.115] or [5.576, 5.576] by set order. Move #562's carry-over sort key into _latest_input_key and use it for the uprating source too: on a tie, the input that ends last, then the larger unit, then the period's string form. Distinct periods never tie, so the source depends only on the stored inputs. The factor still runs from the source's start, and the candidates are unchanged (own unit, starting before the period, inputs only). Tests: regressions for both set orders with and without a set_input helper, monthly inputs, a 10-year input whose string sorts first, memory versus disk and a branch input; Hypothesis properties for set-order and disk-placement independence and for flat-index uprating agreeing with auto-carry-over; the fixture's reference rule breaks ties the same way. Review (GPT-6.1 Sol, f051cc2): the key's period.stop raised OverflowError for a period ending after 9999-12-31 (day:9999-12-30:3), where the start-only key did not. The key now orders ends with #562's _end_order, which puts such a period last; regressions use a daily variable. The flat-uprating differential now skips only the case where the two paths really diverge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
force-pushed
the
fix-uprating-equal-start
branch
from
October 5, 2026 15:55
2e00515 to
fcd4c4a
Compare
MaxGhenis
force-pushed
the
fix-uprating-equal-start
branch
from
October 5, 2026 22:58
3f222e1 to
578911f
Compare
OnDiskStorage names a value's file "{branch}_{period}.npy". On Windows,
numpy.save raises OSError for default_year:2012:2.npy and
default_year:2014:3.npy, so storing year:2012:2 or year:2014:3 on disk
fails there (#526). The tie tests put such
periods on disk, which failed the four Windows test jobs: the "longer"
memory-or-disk regression (both systems) and the set-order property.
The fixture's can_store_on_disk says whether the tests may put a period
on disk on this platform. On Windows the "longer" regression is skipped
(the "shorter" one still puts 2012 on disk beside year:2012:2 in
memory), and the property drops periods with a colon in their string
form from its drawn on-disk set, so memory-against-disk ties are still
drawn there, with the winner in memory. Elsewhere nothing changes: the
derandomized draws are the same.
Checked with a plugin that makes numpy.save and tempfile.mkstemp reject
any file name containing ":": without the guard the same 3 tests fail
with the same paths as CI; with it 3 pass and 2 skip.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ng a tie Review of 6148851 (APPROVE WITH NITS): - The guard keyed on sys.platform, so it would not lift when #526 lets OnDiskStorage store a period with a colon in its name. The fixture now tries storing year:2012:2 in a temporary directory once, at import, and DISK_STORES_ANY_PERIOD says whether that worked. - The comments said "a period several units long", but the code checks for a colon in the period's string form (year:2012-03 has one; month:2012-01:12 prints as 2012 and has none). They now say so. - No property example had the winning input on disk and a losing one in memory: in the drawn ties split between memory and disk, the winner was always in memory, which get_known_periods lists first. A new example puts year:2012:2 on disk beside 2012 in memory. Where storage cannot store year:2012:2 it is an expected failure with OSError. There, no tie this property draws can have its winner on disk. Two inputs tie when the key ranks them equal up to their start: the same start, and both in the variable's own unit or both not. This property's inputs are years and months, so the two are in the same unit, and the winner, which ends last, is the longer; every such period it draws has a colon in its string form. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of 3f222e1 (APPROVE WITH NITS): - The fixture said Windows does not allow ":" in a file name, so numpy.save raises OSError there. CI showed that for default_year:2012:2.npy, a name with two colons. The docstrings and comments now name what the probe stores (year:2012:2) and say what the tests do where it fails. - example(...).xfail, which the disk-winner example uses, needs Hypothesis 6.62.0 or later: under 6.61.3 it raises AttributeError, so the module could not be collected with an older Hypothesis the dev extra allowed. The dev extra now requires hypothesis>=6.62,<7, and uv.lock's specifier matches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviews of 578911f (Opus 5.5 and GPT-6.1 Sol, both APPROVE WITH NITS): - The reference rule ordered ends by Period.stop, which raises OverflowError for days that end after 9999-12-31, and gave every such input the same end, after any input whose stop has a value. So two of them fell through to string order, and one beat a month input that ends later (month:9999-01:13's stop is 10000-01-31). The simulation orders them with _end_order. For uprated_daily_flat at 9999-12-31, {day:9999-12-30:10, day:9999-12-30:3} gave [10, 20] from the simulation and [1, 2] from the reference; for carried_any_unit at 9999, {month:9999-01:13, day:9999-01-01:370} gave [1, 2] and [3, 4]. The reference's last_day now numbers each period's last day itself: for days, the start's number plus the days after it; for months and years, stop, moved back by whole 400-year Gregorian cycles (146097 days) to a year datetime has. - Tests: both cases as regressions, in either set order, compared with the reference; and a derandomized property that last_day, written separately, plus one equals _end_order, for days, months and years up to 5,000,000 units long. Against the old reference the regressions fail 6 tests; without the 400-year shift, 3; with last_day off by one, the property fails. - The fixture said that where the probe fails the tests keep every period with a colon in memory. There, the "longer" regression is skipped and the disk-winner example expects the OSError; it now says so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
force-pushed
the
fix-uprating-equal-start
branch
from
October 6, 2026 00:19
578911f to
f081b7c
Compare
Review of f081b7c (GPT-6.1 Sol, APPROVE WITH NITS): the fragment said "two earlier inputs start on the same day", but uprating reads only inputs in the variable's own unit. A yearly variable with no set_input helper, given 2012 and month:2012-01:25, which start together, is still uprated from 2012, though the month input ends later. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
force-pushed
the
fix-uprating-equal-start
branch
from
October 6, 2026 00:49
b7713e4 to
db5c3de
Compare
MaxGhenis
marked this pull request as ready for review
October 6, 2026 01:16
MaxGhenis
added a commit
that referenced
this pull request
Oct 6, 2026
Brings in #562 (carry over only inputs), #563 and #582 (uprate only from inputs), #578 (shared-key sets), #574 (per-call parameter tracing) and #584. One input record instead of two: the storages' `_inputs` (memory) and `_derived` (disk) from #562 replace this branch's `_input_keys`, so an input is a value stored without `derived=True`, as on master. Sequence numbers stay alongside. Disk storage takes master's `_path_to_write` (a clone never writes over a file it shares) in place of this branch's per-store file names, so the process tokens and restore ordering go. Dumps write master's `derived_periods.txt` (branch-aware) and restore inputs first, then calculated values under one later number. `apply_reform` marks every value set_input did not store as derived and drops it, keeping master's semantics without wiping and replaying. Tests that expected a calculated result to carry over now check that the result is cached, since only inputs carry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
added a commit
to MaxGhenis/policyengine-core
that referenced
this pull request
Oct 6, 2026
Conflicts resolved by keeping master's bookkeeping: - holder.py: master's derived put (PolicyEngine#562) and branch helpers; this PR's fast-cache eviction after every storage write and delete. - simulation.py: master's input-only uprating (PolicyEngine#563/PolicyEngine#582); this PR's calculate-time value-type check runs just before an input is uprated. - simulation_dumper.py: both records for now (inputs.txt and master's derived_periods.txt); a value marked derived restores as derived. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
added a commit
to MaxGhenis/policyengine-core
that referenced
this pull request
Oct 6, 2026
Brings in PolicyEngine#562, PolicyEngine#563, PolicyEngine#582 (input/derived storage marks, input-only carry-over and uprating), PolicyEngine#574, PolicyEngine#584 (parameter tracing and copy guards), PolicyEngine#578 (shared keys only while sharing) and PolicyEngine#585 (temporary storage directories). Conflict in holders/holder.py, resolved by keeping master's bookkeeping: - _set keeps master's ``derived`` mark (stored with the value, and a derived value is never redirected to the running set_input's branch) and this branch's ``is_input``, which decides what the record names. - put_in_cache keeps master's guard (a derived value never replaces a readable input) and passes ``is_input=False``. - master's ``Holder._stores`` (storage ``has``) replaces this branch's equivalent ``_stores``. simulation.py merged cleanly but copied the record in ``clone`` twice (PolicyEngine#562 added the same copy); kept master's copy and added the ``_user_input_contexts`` reset next to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
#562 and #563 have merged, so this is now one PR on
master(9a12a9a5). The head isdb5c3de8: this PR's commitfcd4c4a0, rebased onto master, plus five follow-up commits that answer the reviews. Onlyfcd4c4a0changes code underpolicyengine_core/.Found by the final-commit review of #563 (finding 5). The tie is still on master.
Problem
When a variable with
upratinghas no formula result for a period,Simulation._calculateuprates the latest earlier input in its own unit. On master it picks that input withmax(earlier_input_periods, key=lambda p: p.start).Two earlier inputs can start on the same day.
Holder.set_inputcalls a variable'sset_inputhelper only for an input in another unit, so a yearly variable stores ayear:2012:2input as given, beside one for2012. It does this whether or not the variable has a helper. Both inputs start on 2012-01-01, somaxreturned whichever came first inget_known_periods(): the one stored first, or the one in memory over the one on disk.Executed on master
9a12a9a5, with an index rising 3.7% a year and inputsyear:2012:2 = 1and2012 = 5:year:2012:2set first2012set firstuprated_any_unit(no helper)1.11515765.575788uprated(set_input_divide_by_period)1.11515765.575788uprated_monthly,month:2012-01:3vs2012-01, for 2012-061.05.0The results are the same with auto-carry-over on and off.
Fix
Use the order #562 already uses to choose the input auto-carry-over carries. Its sort key moves into one module function,
_latest_input_key(period, definition_period), and both paths call it. The key ranks, in order:_end_order, which also orders periods ending after 9999-12-31);On a tie, uprating now starts from the input that ends last. That is the input auto-carry-over would carry, so uprating with a flat index returns exactly what auto-carry-over returns.
The production diff is that one function and the two
maxcalls that use it. The carry-over path's behaviour is unchanged; only its inline key moved. Two things are deliberately unchanged:start;Why "ends last"
upratingstart from a different input than auto-carry-over carries.year:2012:2says what the variable is in 2013 as well. Auto-carry-over already reads it that way.Should uprating from a multi-period input be allowed at all?
The factor uses
period.start. Uprate ayear:2012:2source to 2015, and two things go wrong:Changing that would change results without any tie, which makes it a semantic choice rather than a determinism fix. It also sits with related multi-period behaviour that isn't about ties:
month:2012-01:12is stored as the year 2012, because storage keys are period strings.They are written up in #583, each one executed, with options and a recommendation. This PR keeps multi-period inputs as sources and only makes the tie deterministic.
Exposure: none known.
2024and2025only.set_input(variable, year, …)for each year indataset.years.period:.year:…:Nstring in either repository's source and tests is a parameter update, not a variable input.PolicyEngine/policyengine-us@fd693d66andPolicyEngine/policyengine-uk@bfc5fea1.Invariants
Over the generated domain:
year:2012:2,year:2012:10,month:2012-01:3and others);ADD,DIVIDE);test_uprating_does_not_depend_on_the_order_inputs_were_set._latest_input_keynever ties on two distinct periods: unit, start and end fix the period. Somaxnever falls back to iteration order.uprating. Tested bytest_flat_uprating_reads_the_input_auto_carry_over_reads.tests/fixtures/uprating_order.py, which is written independently of_latest_input_keyand breaks ties the same way on both paths.last_daynumbers each period's last day without_end_order: for days, the start's ordinal plus the days after it; for months and years,Period.stop, moved back by whole 400-year Gregorian cycles.last_day + 1 == _end_orderfor days, months and years up to 5,000,000 units long, many of them ending after 9999-12-31.Tests
tests/core/test_uprating_order.pygoes from 97 to 141 cases, still with no Hypothesis dependency. The new cases cover:2012,year:2012:2andyear:2012:10, where the string order disagrees with the end;day:9999-12-30:10andday:9999-12-30:3), and a carry-over betweenmonth:9999-01:13andday:9999-01-01:370. Each is compared with the reference rule.tests/core/test_uprating_order_property.pygoes from 2 to 5 derandomized properties:@examplerows;last_daydifferential against_end_order, 500 examples.tests/fixtures/uprating_order.pygains:last_day;uprated_flatanduprated_daily_flat, uprated by an index that never moves, andcarried_any_unit, their twin without uprating;Windows (#526).
OnDiskStoragenames a value's file after its period. On Windows,numpy.saveraisesOSErrorfordefault_year:2012:2.npy, a name with two colons; CI showed this on the four Windows test jobs.year:2012:2in a temporary directory.DISK_STORES_ANY_PERIODrecords whether that worked.xfail(raises=OSError).OnDiskStoragestore such periods, the probe succeeds and all of these run on disk again.example(...).xfailneeds Hypothesis 6.62.0 or later, so the dev extra now requireshypothesis>=6.62,<7.uv.lock's specifier matches. The lock's own project version was already stale on master (3.32.12against3.32.17), and this PR leaves it.Against master's code, 22 of the 447 tests in the five uprating and carry-over files fail: 19 regression cases and 3 properties.
Mutation check on
db5c3de8:test_storage_input_index.py: 465 tests, 0 failing unmutated;numpy.saveandtempfile.mkstempreject a file name containing:, so the probe fails as it does on Windows: 377 pass and 2 are skipped, unmutated.All are killed. Failing tests:
startonly (master's rule)On the reference rule itself, 3 more mutants, run on the overflow regressions and the
last_dayproperty:last_dayfails the property.How often the set-order property reaches a tie. On the 300 examples it runs, a tie decides the source in 76. In the draws it splits between memory and disk, the winner is always in memory, which is why the explicit disk-winner example exists.
Local runs:
db5c3de8:ruff format --check .: clean.f081b7c4, which differs fromdb5c3de8only in the changelog fragment:testsfolder: 1532 passed, 4 skipped, 1 xfailed;1.1151576, with auto-carry-over on and off. On master9a12a9a5it gives5.575788when2012is set first.CI on
db5c3de8passed: all 18 checks in run 37395979986.Review
f051cc28: REQUEST CHANGES.period.stop, which raisesOverflowErrorfor a period ending after 9999-12-31. Fixed: the key uses Carry over only inputs, the latest at or before the requested period #562's_end_order.e12993c5: APPROVE.6148851d, then3f222e1f): APPROVE WITH NITS each time, no code defect. Between them they found that:sys.platform; it is now a probe;example.xfailneeds Hypothesis 6.62. All are fixed.578911f2: both APPROVE WITH NITS, no production defect.last_day, with regressions and the differential property.f081b7c4: two independent GPT-6.1 Sol reviews, one APPROVE and one APPROVE WITH NITS. The nit was that the changelog fragment did not say the tied inputs are in the variable's own unit. Fixed.b7713e4d: two independent GPT-6.1 Sol reviews, one APPROVE and one APPROVE WITH NITS. The nit was that a commit-message example needed "with noset_inputhelper".db5c3de8rewords that message; its tree is identical tob7713e4d's.The rewordings kept every reviewed tree: the reviewers checked
b43c3562,7e2aa238and49ed6a4a, andgit diff --quiet b7713e4d db5c3de8is clean. The commit messages form the squash body, so they were reviewed as well.Not in scope
axiom: n/a: core engine semantics, no policy encoded.
🤖 Generated with Claude Code