Skip to content

Count only supplied inputs that are still stored in their tier - #2308

Draft
MaxGhenis wants to merge 3 commits into
fix-subsample-2254from
fix-supplied-input-tiers
Draft

MaxGhenis wants to merge 3 commits into
fix-subsample-2254from
fix-supplied-input-tiers

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #2307

Stacked on #2265. #2265 adds stored_supplied_value, and this PR builds on it rather than adding a second copy. The PR targets #2265's branch (fix-subsample-2254), so the diff shows only this change. When #2265 merges and its branch is deleted, GitHub moves this PR to main. CI runs only for PRs into main, so it runs then.

What was wrong

supplied_input and supplied_input_periods treated a value cached over a supplied input as supplied. Reproduced on main at 6d89338 with policyengine-core 3.32.27 (the locked version) and 3.33.2. The reproduction is in #2307: after set_input of 100 and put_in_cache of 999 for the same period, supplied_input returned [999.] and supplied_input_periods returned [2025]. Core's Holder._stores_user_input returned False, and on 3.33.2 Simulation._get_recorded_input_array returned None.

Why

Both helpers checked only two things: that core recorded the input (Simulation._user_input_keys), and that Holder._get_array_from_storage returned something. That method reads memory first, then disk, and does not ask whether the value it finds is the input. The record outlives a value cached over the input. put_in_cache stores over the input and takes that tier off the holder's record of where the input is (Holder._user_input_storage), but the _user_input_keys entry stays. So there were two wrong answers:

  1. A value cached over the input in the same tier was returned as supplied.
  2. An input stored on disk, with a value cached over it in memory, was read from memory, so the cached value was returned.

What changes

  • supplied_input reads through stored_supplied_value (from Give UK simulations a working subsample #2265). That reads a supplied value only from a tier that the holder records the input in and that holds a value the engine did not calculate. This is the rule Holder._stores_user_input uses.
  • supplied_input_periods uses the same rule. It finds the tier without reading the value, which may be on disk. To share that step, stored_supplied_value now calls a new private _supplied_value_storage. What stored_supplied_value returns is unchanged.
  • Unchanged:
    • Branch order. supplied_input still looks at this branch first, then each branch it was made from, then the default branch. If a branch's input is gone, it reads the next one, as core's _get_recorded_input_array does.
    • Neutralised variables. A neutralised variable whose input is still stored reads as its default.
    • SUPPLIED_INPUT_VARIABLES and its check.
  • Not changed: drop_missing_supplied_inputs. It forgets a record whose value is gone from both tiers, the population-rebuild case (policyengine-core#605). It keeps a record whose input a cached value has replaced. That is harmless now: the helpers ignore such a record, and so does core's input export (_stores_user_input).

Does this change results?

Not in normal calculation. The defect shows only when code stores a value with Holder.put_in_cache and its default derived=False, or with Holder._set.

  • In policyengine-core 3.32.27 and 3.33.2, Simulation stores every value it calculates with put_in_cache(..., derived=True).
  • put_in_cache with derived=True stores nothing when the value get_array would read for that period is an input.
  • Core's other writers store inputs: set_input and its period helpers, and the input replay in _invalidate_all_caches.
  • The simulation dumper restores a dump into a new simulation, one value for each variable and period, so it stores nothing over an input either.
  • policyengine-uk calls neither put_in_cache nor Holder._set.
  • A code search of the PolicyEngine organisation finds direct put_in_cache calls only in US code: one crfb-tob-impacts worker and two policyengine-us tests.

test_calculating_leaves_every_supplied_value_supplied executes this claim. It supplies every variable in SUPPLIED_INPUT_VARIABLES for two years, on the default branch and on a branch, and calculates ten outputs over five years on both. Then it checks that each input is still supplied and unchanged, and that core still counts every recorded input as stored. This test also passes on the code before the fix.

What the callers ask:

  • utils/bus_fares.py (has_bus_journeys) asks whether any bus journeys were supplied.
  • person_bus_fare_spending asks for the supplied household bus_fare_spending and the supplied journey counts.
  • ni_class_4_trading_loss asks for the supplied trading_loss of the year.
  • ni_class_4_losses_brought_forward asks for the years with a supplied loss or balance, and for the supplied balance.

None of these answers changes unless a value has been cached over the input by one of the routes above.

Invariants

Stated in policyengine_uk/tests/test_supplied_inputs.py. The model covers every sequence of these steps: inputs set, values cached (calculated or not), arrays deleted and calculations run. Each step can act on a branch, on the branch it was made from or on the default branch, in memory or on disk.

  1. Survival (model). supplied_input returns the input on the nearest branch that still stores one for the period, reading memory before disk. Otherwise it returns None. An input stays stored until it is deleted, or until another input or a cached value is stored over it in the same tier. A value cached in memory over an input on disk leaves that input. A calculated value never replaces an input.
  2. Agreement. supplied_input returns a value for a period exactly when supplied_input_periods lists that period, and the list runs from the earliest period.
  3. Differential.
    • For every recorded input, the helpers count it as supplied exactly when core's Holder._stores_user_input does.
    • On core 3.33.1 and later, supplied_input returns what Simulation._get_recorded_input_array returns.
    • On core 3.33.2 and later, supplied_input_periods lists the periods that Simulation._get_set_input_periods lists.
  4. Calculation. After calculation, every supplied value of every registered variable is still supplied and unchanged.

Intended: a neutralised variable whose input is still stored reads as its default.

Tests

  • policyengine_uk/tests/test_supplied_inputs.py has 15 tests:
    • one regression test for each of the seven registered variables;
    • other years' inputs are still read;
    • an input on disk under a value cached in memory is supplied;
    • a value cached over an input on disk is not supplied;
    • a calculated value is never stored over an input;
    • branch order;
    • a neutralised input;
    • the calculation test above;
    • a Hypothesis property test (100 examples, five explicit ones) of a model over a three-branch chain and both tiers.
  • Hypothesis found an error in the model, not in the fix. From core 3.33, set_input first drops cached values that may depend on the input, so an input can go to disk where the model expected memory. The model now records the tier each value is actually stored in, and the case is pinned as an explicit example.
  • Run locally, in one process with a fresh --basetemp:
    • Core 3.32.27: the new tests, test_bus_fare_reforms.py, test_input_records.py, test_ni_class_4_profits_properties.py and test_subsample.py gave 103 passed and 1 skipped. The skip is Give UK simulations a working subsample #2265's 3.33-only differential.
    • Core 3.33.2: the same files gave 104 passed.
    • The YAML policy tests that reach the helpers passed on both cores, 66 cases in each run: bus_journeys, person_bus_fare_spending, loss_relief, ni_class_2 and ni_class_4_profits.
  • Mutation check, on both cores:
    • The new tests against the code before the fix: 13 of 15 fail. The two that pass are the calculation tests, which should hold before the fix too.
    • Four mutants of the fix are each caught: no fall-back to the parent branch, the periods helper left on the old rule, ignoring the recorded tiers, and reading disk before memory.
  • uvx ruff format --check . and uvx ruff check . pass with ruff 0.17.0, the latest release.
  • Not run locally: the full suite. CI runs it once this PR moves to main. On 2026-10-11, the Test job of Give UK simulations a working subsample #2265 failed only in test_dataframe_dataset.py::test_an_exported_frame_loads_back_and_exports_itself, which fails on main too, and then the runner was shut down.

axiom: n/a: simulation infrastructure, no policy rule change

🤖 Generated with Claude Code

MaxGhenis and others added 3 commits October 11, 2026 11:45
supplied_input and supplied_input_periods checked only that core recorded
the input and that Holder._get_array_from_storage returned something. That
reads memory before disk and does not ask whether the value is the input,
so a value cached over a supplied input counted as supplied, and an input
on disk under a value cached in memory was read from memory.

Both now use the tier rule of stored_supplied_value (Holder's
_stores_user_input), keep the nearest-branch-first order, and keep the
neutralised-variable behaviour. Fixes #2307.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Example tests for a value cached over an input in memory and on disk, an
input on disk under a value cached in memory, branch precedence and a
neutralised variable. A Hypothesis model of inputs set, values cached,
arrays deleted and calculations run over a three-branch chain and both
tiers checks the helpers against the model, against each other, and
against policyengine-core's Holder._stores_user_input and (from 3.33.1)
Simulation._get_recorded_input_array. A last test calculates with every
registered variable supplied and checks each input is still supplied, so
normal calculation never hit the defect.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On policyengine-core 3.33.2 and later, the property test also checks that
the periods the helper lists are the periods core lists.

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