Skip to content

Cache ADD and DIVIDE results only where a plain read returns them - #571

Draft
MaxGhenis wants to merge 1 commit into
masterfrom
fix-stock-option-caches
Draft

MaxGhenis wants to merge 1 commit into
masterfrom
fix-stock-option-caches

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

calculate_add and calculate_divide stored their result at the requested period, and every later plain calculate of that variable and period returned it. That is the right value for a FLOW variable (its plain value over a period of another unit is exactly that sum or twelfth), but not elsewhere. So a plain read returned a different value depending on whether the option had run first:

Case (executed on master b78b0ba) New simulation After the option
Monthly STOCK float, input 10 in 2012-01 (carry-over on): calculate(var, "2012") after calculate_add(var, "2012") 10 120
Yearly int (STOCK by default), input 12 in 2012: calculate(var, "2012-06") after calculate_divide(var, "2012-06") 12 1
Yearly variable, calculate(var, "year:2012:2") after calculate_add over it ValueError the sum
Day variable, calculate(var, "2012-01") after calculate_add over the month its monthly value the 31-day sum
Monthly FLOW variable with an input stored at the year (set_input = None): calculate(var, "2012") after calculate_add the input the sum (input overwritten)
Yearly int FLOW variable, second calculate(var, "2012-01") 84.33 84 (cached truncated)

Found while reviewing #562 (soundness review, finding 5).

The rule

Simulation._cache_option_result stores an ADD or DIVIDE result only where _calculate itself routes a plain read to that option (a FLOW variable read over a period of another unit), only when the result already has the variable's dtype, and never over a value a plain read already finds there. Everywhere else the option result is returned but not cached. _calculate's routing is unchanged, so FLOW reads keep their cache.

Invariants

  • For any variable, any inputs and any sequence of calculate, calculate_add and calculate_divide requests, a request returns the same values (or raises the same error) as in a new simulation with the same inputs. Property test: tests/core/test_option_result_cache_property.py (Hypothesis, 400 examples over MONTH/YEAR, STOCK/FLOW, float/int/bool, with and without formulas and set_input; auto-carry-over off, because carrying over is a separate order dependence fixed by Carry over only inputs, the latest at or before the requested period #562). It fails on master within seconds.
  • An option never replaces a value already readable at its period.

Downstream impact

Real runs on master b78b0ba and on all three of #571/#572/#573 merged together (scratch merge 37aadada), each output compared array by array (compare.py). The PE-US 2026 run was also repeated on each branch alone, and the 2035 run on #572 alone; all identical too.

Run Outputs compared Result
policyengine-us 6c5170fd, eCPS 2024, 3,000-household subsample, 2026 (income tax, itemizing and SALT branches, state taxes, CTC, EITC, SNAP, household and SPM net income, weights, marginal tax rates) 25 arrays bitwise identical
same, 2035 25 arrays bitwise identical
policyengine-uk 7b9fc379, enhanced FRS 2024-25, full sample, 2026 (income tax, NI, UC and legacy benefits, Pension Credit, HB, council tax, HBAI income, poverty, marginal tax rates) 36 arrays bitwise identical

Neither policyengine-us nor policyengine-uk formulas use the ADD or DIVIDE options (git grep on main), so nothing they calculate goes through the changed caching.

Tests

  • tests/core/test_option_result_cache.py: 12 regressions; 10 fail on master, and the other 2 pin the FLOW caching that is kept.
  • tests/core/test_option_result_cache_property.py: the property above, behind pytest.importorskip("hypothesis") for the smoke job.
  • Mutation check: removing any of the four guards (STOCK, routing, dtype, already stored), or never caching, fails the tests (5/5 killed).
  • Full suite: 1156 passed, 4 skipped, 1 xfailed.

Composition

Touches calculate_add/calculate_divide, which #562 also edits (it adds derived=True and skips single-sub-period sums). The conflict is textual: keep #562's derived=True inside _cache_option_result. The rule here already never caches a single sub-period's sum.

axiom: n/a: core engine caching, no policy encoding

🤖 Generated with Claude Code

calculate_add and calculate_divide stored their result at the requested
period, where every later plain calculate of that variable and period
found it. For a STOCK variable that is not its plain value (last month
of the year, or the year's value for a month), so a monthly STOCK read
120 instead of 10 after an ADD and a yearly one 1 instead of 12 after a
DIVIDE. The same happened over several periods of a variable's own unit
(a plain read raises), for day variables, for integer or boolean FLOW
results the cache truncates, and over an input stored at that period.

Results are now cached only for the case _calculate itself routes to
these options (a FLOW variable over a period of another unit), when the
result has the variable's dtype and nothing is stored there yet.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant