Stop formulas writing into arrays they read from other variables - #9739
Conversation
policyengine-core returns the cached array itself from
tax_unit("x", period), so `x += y` in a formula changed variable x's
cached value for every later reader. Five formulas did this:
- reforms/crfb/agi_surtax.py: adjusted_gross_income (increased base)
- ga_deductions.py: itemized_taxable_income_deductions
- ut_personal_exemption.py: ut_total_dependents
- ut_dependent_exemption_reform.py: ut_total_dependents
- basic_income_phase_in.py: tax_unit_earned_income (SS as earnings)
Each now builds a new array. nj_gross_income and
ma_ccfa_countable_income_person wrote into the result of
add(entity, period, <parameter list>), which is the cached array itself
when the list holds one variable; their lists hold 2+ today, so this is
a no-op guard. A code-health test scans variables/ and reforms/ for new
in-place writes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
One test per in-place write: calculate the consumer, then check the source (adjusted_gross_income under the CRFB surtax, the federal itemized deductions in Georgia, ut_total_dependents in baseline and under the dependent exemption reform, tax_unit_earned_income under the basic income phase-in). A last test calculates three kinds of household in every state and DC with every cached array read-only, under the baseline and each reform, so any formula on that path that writes into a cached array raises at that line. All fail on main before the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9739 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 5 +1
Lines 76 90 +14
Branches 2 5 +3
=========================================
+ Hits 76 90 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Scanner: walk the sides of an if, the cases of a match and the parts of a try from the same incoming state and merge them, and walk loop bodies as if they ran zero, one or two times, so an optional copy on one path no longer hides a write on another. Recognise writes through views (x.reshape(-1)[:] += 1, np.copyto(x[:], ...), out=x[:], x.flat[...]), entity calls with a variable name held in a name, and stop flagging x.byteswap() without inplace=True. Read-only test: freeze cached arrays as they are read, not only as they are stored, so arrays a branch inherits (InMemoryStorage.clone copies them without put) are covered; test that directly. Add a case for MA CCFA with a one-variable earned list, where add() returns the cached array and the old subtraction overwrote self_employment_income. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Parametrize the Utah test over the newborn credit being off, the basic income test over Social Security counted or not and per-person phase-in, and the MA CCFA test over parent-only counting and the minor earnings exclusion, so each changed formula's branches all run and the source variable is checked on every path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged with |
Summary
policyengine-core returns the cached array itself from
tax_unit("x", period)(Simulation._calculatereturnsholder.get_array(...)), andadd(entity, period, sources)returns that same array whensourcesholds a single same-entity variable (for_each_variablestarts from the first variable's values). A formula that writes into it (x += y) therefore changes variablex's cached value for every later reader in the simulation, and inside a branch it changes the branch's copy. Five formulas did this:reforms/crfb/agi_surtax.pyadjusted_gross_income(+ the increased-base sources)increased_base.in_effectga_deductions.pyitemized_taxable_income_deductions(− the Georgia adjustment)ut_personal_exemption.pyut_total_dependents(+ newborn dependents)ut_dependent_exemption_reform.pyut_total_dependentsbasic_income_phase_in.pytax_unit_earned_income(+ Social Security)include_ss_benefits_as_earningsEach now builds a new array (
x = x + y). What changes is every variable calculated after them from the overwritten one. The formulas' own outputs are unchanged with one exception:agi_surtaxnow sums its base in float64. The oldagi += …rounded AGI plus the extra sources to float32 before applying the rates (float32 spacing is $1 at $10 million and grows with AGI). In the 2026 run this moves 3 records by at most $0.02. For a single filer the gap grows with AGI: the review's NumPy emulation of the schedule puts it at up to $0.125 for AGI of $10m–$100m and $2 for $100m–$1bn.nj_gross_incomeandma_ccfa_countable_income_personwrote into the result ofadd()over a parameter list. Those lists hold 13 and 14+ variables in every dated value, so the write never reached the cache. They now build new arrays too, so a later edit of either list to a single variable cannot turn them into cache writes. With the current lists their outputs don't change (measured below). With a one-variable MA earned list, the old subtraction also overwrote the formula's ownearnedlocal (the same cached array). The new code gives the formula's result instead. A test checks that the source variable is no longer overwritten; it does not pin the formula's result. Separately, if a reform turned on the minor-earnings exclusion with parent-only counting off, the dependent path would subtract a minor's earnings twice. Baseline never combines the two (the exclusion starts in 2026; parent-only counting has applied since October 2023), and this PR doesn't change it: #9759.Tests
tests/test_formulas_do_not_write_into_cached_arrays.py(17 tests). One test per site calculates the consumer, then checks the source variable: AGI stays $400,000 afterincome_taxunder the CRFB reform; New York state income tax under the reform equals baseline; Georgia's federal itemized deductions are unchanged afterhousehold_net_income;ut_total_dependentsstays 2 for a newborn plus a child, in baseline and under the dependent-exemption reform; earned income stays $10,000 under the basic-income phase-in; MA'sself_employment_incomestays put with a one-variable earned list. A final test calculates three kinds of household in every state and DC under baseline, the CRFB reform, the Utah reform and the basic income. Every cached array is read-only, both as stored and as read, which also covers the arrays branches inherit. Any formula on that path that writes into a cached array then raises at that line. A further test checks that this fixture catches a write into a branch's inherited array. The per-site tests are parametrized over each formula's branches: Utah's newborn credit on and off, Social Security counted or not, per-person phase-in, and MA's parent-only rule and minor exclusion. That way every line this PR changes runs, along with every branch ofut_personal_exemption,basic_income_phase_inandma_ccfa_countable_income_person. The Utah reform's in-effect branch, which this PR doesn't touch, is not exercised here. 13 of the 17 fail on main; all pass here. The 4 that pass on main are the three paths where the old code never wrote, plus the fixture test.tests/code_health/test_no_in_place_writes_to_cached_arrays.py: an AST scan ofvariables/andreforms/that fails CI on new writes into arrays read from variables. It catches augmented and subscript assignment,np.place/putmask/copyto,ufunc.at,out=, and in-place methods such as.fill()and.sort(), including writes through views such asx.reshape(-1)[:] += 1. It tracks the result of entity calls (with a literal or variable name),.members(...),.calculate(...), andadd()over anything but a literal list of two or more variables, through aliases, views, conditional expressions and tuple unpacking. The sides of anif,matchcases andtryblocks start from the same state and merge, and loop bodies are walked as if they ran zero, one or two times. So an optional copy on one path doesn't hide a write on another. On main it reports the five sites plus the four NJ lines and the MA line; here it reports none, across the whole package. It is not exhaustive. Positionalout, alias chains that need three or more loop passes, walrus bindings, closures and some views escape it. None of those appears in the codebase; Harden the in-place cached-array write scan (positional out, loop fixed point, walrus, more views, tools/) #9760 tracks them. It has its own positive and negative self-tests. The scan from the original audit also flaggedthree_digit_zip_code.py, which writes into a new pandas Series; this one doesn't.sitecustomize.pyonInMemoryStorage.putand.getthat logs any read-onlyValueErrorraised inside a formula.What changes (real microsimulation runs)
Default dataset (
populace_us_2024.h5@populace-us-2024-spm-20260909), 2026, oneMicrosimulationper run. Each run calculateshousehold_net_incomefirst, then reads the other variables from the same simulation, comparing main (20ccd5ac7c) with this branch. Scripts and outputs:impact.py,compare.py,out/compare.md.Baseline (current law). Every tax, benefit and net-income total is identical. The only output that moves is the reported
ut_total_dependents: 1.165m on main vs 1.127m here. On main, newborn dependents were added into it a second time; the Utah exemption itself was always right. 41 records (0.04m weighted tax units) differ. Georgia's adjustment is an input the dataset does not carry (zero for every record), so no Georgia output moves in microsimulation. The household case is fixed and tested.CRFB AGI surtax with the increased base (reform minus baseline):
The surtax itself is unchanged. On main, everything calculated from AGI after
agi_surtaxused AGI plus the increased-base sources (AGI totalled $20,400bn on main vs $18,931bn). That covers state income taxes, federal credits calculated after the surtax, and the premium tax credit. The state correction is largest in CA ($7.66bn), IL ($3.09bn), MD ($1.84bn), MN ($1.83bn) and NC ($1.73bn). On main the reform appeared to cut net income in the bottom half of the distribution ($21 to $224 per household in deciles 1–5). Fixed, it is about zero there, as the surtax starts at $100,000 / $200,000.Utah dependent exemption reform, installed but not in effect in 2026 (
in_effectfrom 2027, so the 2026 run uses the reform's formula, checked): same as baseline. Onlyut_total_dependentsmoves (1.165m → 1.127m); no tax or income output changes.Basic income ($12,000 flat) phased in with Social Security counted as earnings: the basic income itself is unchanged. On main the reported
tax_unit_earned_incometotal was $1,677.9bn too high (39.95m weighted tax units differ). One later reader picked it up: state income tax falls $0.011bn here (8 records), and household net income rises by the same amount.Runs of the CRFB increased-base option before this fix (it was added 2026-01-23) had the same bug: everything calculated from AGI after the surtax used the inflated AGI, as in the main-branch column above.
Invariants
agi_surtax's float32-to-float64 rounding (see above). An independent review executed old and new formula versions on vectors including negatives, cents, large float32 values and int32 counts, with identical outputs for GA, both Utah sites, the basic income, NJ and MA. The per-site tests check the formula outputs (agi_surtax,ga_deductions,ut_personal_exemption,basic_income_phase_in) alongside the sources.axiom: n/a: fixes cache aliasing in formulas; no rule change
Follow-ups: #9759 (MA CCFA double subtraction, pre-existing), #9760 (scan gaps).
Reviews: two rounds by an independent agent (GPT-6.1 Sol via Subfleet). Round 1 requested changes; they were fixed in fd20f55 or answered in the response. Round 2 approved the head 95d9736.
Related: PolicyEngine/policyengine-core#556 (branches share cached arrays and copy on first read) deliberately keeps today's behaviour for these formulas, so this fix is independent of it.
🤖 Generated with Claude Code