Skip to content

Delete the periods within a deleted period from disk storage - #565

Draft
MaxGhenis wants to merge 3 commits into
masterfrom
fix-disk-delete-contained-periods
Draft

MaxGhenis wants to merge 3 commits into
masterfrom
fix-disk-delete-contained-periods

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #564.

Stacked on #552. This branch carries #552's two commits; review only the last commit, bfa78cb. I'll rebase onto master once #552 merges. It uses #552's _split_key (split on the last _) instead of a second key parser.

What was wrong

Holder.delete_arrays(period) documents that it removes "all values for any period included in period". InMemoryStorage.delete does that. OnDiskStorage.delete(period, branch_name) removed only the file keyed exactly f"{branch_name}_{period}". So a disk-backed simulation (one with a MemoryConfig) kept values the caller had deleted. Here is the issue's reproduction with delete_arrays("rent", "2025"):

in memory on disk
before (#552 head) [] ['2025-01', '2025-02']
this PR [] []

Change

OnDiskStorage.delete(period, branch_name) now deletes each file key whose parsed branch is branch_name and whose period period.contains(...). This is the same rule as InMemoryStorage.delete. Eternal storage still deletes the branch's single ETERNITY key, and delete(None, branch_name) keeps #552's whole-branch-name match. As before, delete only forgets the mapping and leaves the .npy file in place.

Invariants (tested)

  1. Memory and disk agree. For any sequence of put, delete(period, branch) and delete(None, branch), eternal or not, both storages hold the same (branch, period) keys with the same values. This is a differential test, checked after every step.
  2. Deletion follows the documented rule. After delete(p, b), branch b has no key whose period p contains, and every other key is unchanged. After delete(None, b), branch b has no keys. Both storages are checked against a reference model of this rule after every step.
  3. Branches are matched by their whole name. A branch name that contains _, or that starts with another branch's name plus _ (pre / pre_tcja / pre_tcja_ctc, y / y_2019, trailing_), never loses or keeps another branch's keys.

Tests

Open PRs that touch the same code

Each was merged with this branch on top of master and the full suite run:

PR merge full suite
#561 (579c375) clean 1,766 passed
#558 (a9a0b40) clean (on_disk_storage.py composes) 1,767 passed
#562 (912a4ac) simulation_dumper.py conflicts with #552, not this PR. Resolved as #562's body says (#552's branch passed to is_derived) 1,792 passed; its _derived.intersection_update sits after this PR's comprehension
#557 (97d4582) clean 1,979 passed

#561: its disk tests compare the record with what storage still holds, so they pass before and after this fix, and its Holder._forget_deleted_inputs drops an entry only when neither storage still holds the value. One sentence in that method's docstring goes stale once both are merged: "disk storage deletes only the period it is given, not the periods within it (policyengine-core#564), so a value deleted from memory can survive on disk, and its entry stays." Whichever of #561 and this PR merges second edits it.

Impact

Holders get disk storage only when simulation.memory_config is set (Holder.__init__). policyengine-us (4e4999a3), policyengine.py (6a9c878), policyengine-canada, -il and -ng never set it, and policyengine-uk (c7e826ea) sets it to None. So this changes no country-package or policyengine.py result.

Not run: make documentation, and Windows locally (CI covers Windows).

axiom: n/a: core storage fix, no policy encoded.

🤖 Generated with Claude Code

MaxGhenis and others added 3 commits September 27, 2026 15:50
…g over

Holder.get_known_periods() lists the periods of every stored key, with the
branch name stripped, but Holder.get_array() reads only the requested branch,
its parent_branch ancestors and "default". Simulation._calculate took the
latest known period from the unscoped list, so a period stored only under an
unrelated branch read back as None: uprating raised TypeError and
auto-carry-over cached NaN.

- Holder._readable_branch_names() is the one definition of what a branch can
  read; get_array() and the new get_known_periods(branch_name) both use it.
- _calculate uses get_known_periods(self.branch_name).
- OnDiskStorage splits "<branch>_<period>" keys on the last "_": branch names
  like "no_salt" raised ValueError, "y_2019" listed the wrong period, and
  delete(None, "pre_tcja") also wiped "pre_tcja_ctc".
- dump_simulation saves the values the dumped branch reads instead of reading
  every period under "default" and saving None.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The string form of a multi-unit or rolling-year period contains ':',
which a Windows file name cannot. On windows-latest, numpy.save raised
OSError for month:2025-01:3 and year:2024:2, and year:2025-03 read back
until restore() found no file for it. Disk storage has never kept these
periods on Windows; this PR does not change that, so the test skips them
there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
OnDiskStorage.delete(period, branch_name) removed only the file keyed
exactly f"{branch_name}_{period}". InMemoryStorage.delete and the
Holder.delete_arrays docstring remove every period the given period
contains, so a disk-backed holder kept values the caller had deleted
(deleting "2025" left "2025-01" and "2025-02"). Parse each key with
_split_key and delete those whose branch is branch_name and whose period
the deleted period contains. Eternal storage still deletes its one
ETERNITY key.

Fixes #564.

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.

OnDiskStorage.delete(period) deletes only that exact period, not the periods within it

1 participant