Conversation
…ct non-numeric uprating and defined_for Three bugs found by the review of #563, all present on master b78b0ba: - restore_simulation put every value back with put_in_cache, which records no input, so apply_reform on a restored simulation dropped its inputs. dump_simulation now lists each variable's input periods in inputs.txt and restore_simulation records exactly those in _user_input_keys. - Holder.set_input, put_in_cache and delete_arrays left the simulation's fast cache untouched, so calculate kept returning the replaced value. Every holder write and delete now drops the entries it changes, in the holder's own simulation and only for branches that simulation reads. - An Enum, str or date variable with uprating, and a variable defined_for one, raised TypeError in the middle of a calculation. Both are now rejected when the variables are registered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A dump written before inputs were recorded holds the arrays and nothing else, so the test stays right when another change adds its own sidecar. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Next to delete_arrays, git merged it with other changes to that method without a conflict but left their lines inside the new helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hat was calculated from it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ime checks, legacy-dump warning - Test that an input set on a branch (which shares the input record) does not mark the default branch's calculated value as an input, with a branch step in the restore property. - replace_variable keeps the existing variable when the replacement is rejected. - calculate repeats the uprating check before it uprates, for an uprating assigned on a class that declares uprating itself; the defined_for message is tested for str and date as well as Enum. - The message for an inherited uprating points to replace_variable. - restore_simulation warns when a dump does not record its inputs. - Changelog: say that such systems no longer load, including a group variable defined_for a person Enum, which master masked on summed indices. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…is shared policyengine-core#561 makes a branch copy the input record instead of sharing it; the test now holds either way. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
added a commit
that referenced
this pull request
Oct 3, 2026
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>
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.
Three bugs found by the review of #563 (uprating order). None comes from #563: each reproduces on
masterb78b0ba.Draft: this changes core behaviour, so the merge decision is Max's.
The bugs, on
masterand heremaster[1001, 77]for an uprated int variable in 2012; calculate 2013;dump_simulation;restore_simulation;apply_reformwith a reform that changes nothing; calculate 2015[0, 0](the 2012 input is gone too)[1116, 85], as in a new simulationsimulation.get_holder(name).set_input(period("2013"), [300, 400]); calculate 2013 again[1038, 79]while the holder holds[300, 400][300, 400]uprating, input for 2012, calculate 2015TypeError: Forbidden operation. The only operations allowed on EnumArrays are '==' and '!='.ValueErrorwhen the variable is registereddefined_fornames an Enum variable on the same entity; calculateTypeErrorValueErrorwhen the second of the two variables is registeredTwo configurations ran on
masterwithout an error and are now rejected as well (both executed onmaster):defined_fora person Enum. Mapping to the group sums the members' Enum indices, somastermasked on that sum: a household was kept if any member's value was not the Enum's first member ([20.0, 0.0]for one household with apresentmember and one without). This branch raises at registration.upratingthat is only ever read where it has an input. It never reached the multiplication onmaster. This branch raises at registration.No variable in policyengine-us, -uk, -canada, -il, -ng or -au is in either case (see "Country packages").
Fixes
1.
restore_simulationrestores inputs as inputsapply_reformkeeps the values recorded inSimulation._user_input_keysand drops everything else.restore_simulationput every value back withput_in_cache, which records nothing, so the nextapply_reformdropped the restored inputs.dump_simulationwritesinputs.txtnext to each variable's arrays: the periods, one per line, whose dumped value the record names as an input. The record is read the way_invalidate_all_cachesreads it back through storage, so an ETERNITY variable's one value is an input whatever period its entry names.restore_simulationstores exactly those periods as inputs (throughHolder._setinside a_user_input_contextsentry, the pathset_inputuses, so they land in_user_input_keys), and every other value withput_in_cache, as before.inputs.txt(written by an earlier version) does not say which values were calculated. Every value in it is restored as an input, andrestore_simulationwarns. Before,apply_reformdropped every value in it; now it drops none, so the inputs survive, but a reform applied afterwards does not recalculate the values the dump had calculated. That is the rule Make set_input on a branch drop values calculated from the input it replaces #560 and Carry over only inputs, the latest at or before the requested period #562 use for such dumps. The alternative, treating only formula-less variables' values as inputs, would recalculate under a reform but lose any input that had been set on a formula variable.to_input_dataframereads the same record, so a restored simulation now exports its inputs (it exported no input variable before).2. Holder writes and deletes drop the fast-cache entries they change
Simulation.calculateanswers a repeated request from_fast_cachebefore it reads the holder. OnlySimulation.set_inputandSimulation.delete_arraysdropped an entry, and only the exact(variable, period)key.Holder._set(every write:set_input, each store aset_inputhelper makes,put_in_cache) andHolder.delete_arraysnow callHolder._evict_fast_cache:Scope. Only the fast cache of the holder's own simulation. A branch has its own holders and its own fast cache and keeps the values it started with. Nothing is dropped when the write is under a branch name that simulation does not read (
Simulation._get_visible_branch_names).A write drops the entry for the period written, or every entry of the variable if it is defined for ETERNITY (one stored value answers every period).
A delete drops the entries for every period the deleted one contains, or all of the variable's entries when no period is given: the same rule the storage uses.
Cost. A write is a dict lookup, and returns at once when the key is not cached, which is the case for every store
calculateitself makes (it stores, then caches). Only deletes and ETERNITY writes scan the cache. Measured in the policyengine-us A/B run below (withmarginal_tax_rate), eviction took 0.085 s of a 69 s run:User CPU for the whole run: 61.45 s on
master, 61.11 s here.3.
upratinganddefined_forneed values that support arithmeticUprating multiplies the earlier value by an index ratio;
defined_forkeeps values where another variable is> 0. Enum arrays allow only==and!=, andstrand date arrays cannot be multiplied by a float or compared with a number (strand date reproduce the same way onmaster).upratingon an Enum,stror date variable raisesValueErrorinVariable.__init__, next to the computation-mode check, and in theupratingsetter (policyengine-us assignsvariable.upratingafter loading). It covers anupratinginherited from a baseline variable, since that is whatcalculateuses; in that case the message points toreplace_variable, becauseupdate_variablecannot drop an inherited attribute.bool,intandfloatare unchanged.defined_formust name abool,intorfloatvariable.TaxBenefitSystem.load_variablechecks both directions (the variable's owndefined_for, and every registered variable defined for it), so whichever of the two is registered second raises, in any load order. While__init__loadsvariables_dir, the checks are left to one pass over every variable at the end; that only saves a scan of the variables for each non-numeric one loaded.defined_for = StateCode.CAnames the variableCA.calculaterepeats both checks with the same message, for what registration cannot see: adefined_forassigned or a variable put intosystem.variablesafterwards, and anupratingassigned on a class that declaresupratingitself (such a class replaces the property that checks assignments).replace_variablekeeps the existing variable when the replacement is rejected. It deleted the old one first, so a rejection would have left the system without the variable.I did not require a strict
bool: policyengine-us has 14 variables whosedefined_fornames afloat(12) orint(2) variable.Invariants
calculaterequests gives the same bytes (or the same error) on the restored and the dumped simulation, before and afterapply_reformwith a reform that changes nothing. After that reform both also equal a new simulation given only the inputs, except in one existing case (see "Not in this PR").calculatereturns. For random sequences ofcalculate, holderset_input/put_in_cache/delete_arrays(under the simulation's own branch name,default, or a branch it does not read),Simulation.set_input,Simulation.delete_arraysand branch creation, every result equals that of a twin tree whose fast caches are emptied before eachcalculate; so does every stored value at the end.uprating, and nodefined_fornaming a registered non-numeric variable, whatever order the variables were added in.Tests
54 new tests; on
masterb78b0ba 39 fail and 15 pass. The 15 are the cases that must keep working: numericuprating(3) anddefined_for(4), the Enum-member form, an unregistereddefined_for, an Enum input withoutupratingcarrying over, a variables directory with aboolcondition, a dump with an input record restoring without a warning, and three fast-cache guards (a write under a branch the simulation does not read keeps the entry; branch and parent do not touch each other;calculate's own store keeps its entry).tests/core/test_restore_input_registry.py(9, including an input set on a branch, which shares the record the dump reads, and the warning for a dump without a record) andtest_restore_input_registry_property.py(invariants 1 and 2, 200 examples, with a branch-input step).tests/core/test_holder_write_fast_cache.py(10) andtest_holder_write_fast_cache_property.py(invariants 3 and 4, 500 examples).tests/core/variables/test_non_numeric_uprating_and_defined_for.py(33): every non-numeric type, both registration orders, a variables directory in both load orders, the setter and its rollback, a reform that turns an uprated or adefined_forvariable into an Enum, a group variabledefined_fora person Enum, a rejectedreplace_variable, and thecalculatechecks for every non-numeric type.tests/fixtures/uprated_inputs.py. The property modules start withpytest.importorskip("hypothesis").calculatechecks, noreplace_variablerollback), each run against the new tests plustest_fast_cache.pyandtest_dump_restore.py: 36 killed.Country packages
No variable in any PolicyEngine country package is rejected. policyengine-us, -uk and -canada were loaded at their canonical heads with this branch's core:
upratingdefined_fortargetsfloatbool, 12float, 2intfloatboolboolpolicyengine-il (cf3c7fc), -ng (31182d5) and -au (93ae379) have about 20 variables each, none with
defined_fororuprating(static scan at their canonical heads).In a policyengine-us load the new checks took about 17 ms of 20 s (13,276
upratingchecks, 2 passes over the variables, 128 single-variable checks).Single-year outputs are bitwise identical to
master(same country code and data on both arms, core b78b0ba against this branch at 1c24abd, whose production code the later commits do not change):marginal_tax_rate; income tax £313.139bn on both.marginal_tax_rate, which branches per adult and writes and deletes through holders): 25 of 25 arrays identical; income tax $1,962.456bn on both.~/reviews/core-restore-cache-enum-2026-10-02/ab/(run_ab.sh,compare.py, outputs inout/).Relation to open PRs
Each was merged with this branch in a scratch worktree and the full suite run on the result.
Simulation.delete_arrays,set_inputand the spiral purge; this PR changes the holder, which those all go through. So Drop every contained period from the fast cache when deleting or setting a period #566's 9 tests also pass on this branch without Drop every contained period from the fast cache when deleting or setting a period #566's production change. Merging both is safe: both only drop fast-cache entries, and anything Drop every contained period from the fast cache when deleting or setting a period #566 drops on top (a contained period the holder did not write) is read again from storage. Drop every contained period from the fast cache when deleting or setting a period #566's tests are worth keeping either way.Holder._set(keepderived=and the eviction call), at the end ofHolder(both add methods; keep both) and in the dumper, where both sidecars are written and read: a value is restored as an input ifinputs.txtlists it andderived_periods.txtdoes not, and a dump with onlyderived_periods.txtrestores every value it does not list as an input. 1,254 pass._dump_holderonly. The resolution reads the input record for the branch that actually stores the dumped value. With it, dumping a branch that set its own 2013 input restores to[2151, 190]for 2015, equal to a new simulation; on this branch alone that case still gives[1116, 85], becausemasterdumps the default branch's values. 1,729 pass._user_input_keysdrift): conflict inHolder._setonly (keepis_inputand the eviction call). Keep the record of set_input values in step with storage #561 makes a branch copy the input record instead of sharing it; this PR's branch-input test checks the outcome, so it holds either way. 1,228 pass.set_inputinvalidation): it also writesinputs.txt, in the same format, and restores inputs through its per-array flag, so its dumper replaces this PR's;Holder._setkeeps both. This PR's restore tests pass on Make set_input on a branch drop values calculated from the input it replaces #560's dumper once itsrestore_simulationalso warns about a dump withoutinputs.txt(a dozen lines, incompose/560_simulation_dumper.py). 1,280 pass.The resolved files are in
~/reviews/core-restore-cache-enum-2026-10-02/compose/.Not in this PR
set_inputhelpers count a calculated month as already set. Found by this PR's property test, onmaster: calculate one month of a monthly variable, then set an annual input, and the input is divided by 11 instead of 12 ([109.09]instead of[100]); a stock variable gives[0]instead of[7]. The property leaves that case out of its comparison with a new simulation. Chip task_fd52e7d5.Checks run
uv run pytest tests: 1,197 passed, 4 skipped, 1 xfailed (Python 3.14 free-threaded build, numpy 2.4.2, Hypothesis 6.168.3).policyengine-core test policyengine_core/country_template/tests -c policyengine_core.country_template: 39 passed.ruff format --check .andruff check .: clean.master, thereplace_variableloss, the setter bypass).make documentation(no docs page changes; therestore_simulationAPI page is generated from its docstring).axiom: n/a: core engine, no policy change
🤖 Generated with Claude Code