Repository navigation
Conversation
…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 was referenced Oct 2, 2026
MaxGhenis
added a commit
that referenced
this pull request
Oct 7, 2026
MaxGhenis
added a commit
that referenced
this pull request
Oct 7, 2026
Fixes the two findings of #565's review (REQUEST_CHANGES at bfa78cb): 1. An input for a month or year anchored mid-month (month:2025-03-15) was accepted on disk under the calendar month's key, since its string form drops the day; deleting its own period, which does not contain that calendar month, then left it. In-memory storage already rejected it. Both storages now run one check, storage_keys.check_storable, so whether an input is accepted no longer depends on which storage Holder._set picks by memory use; disk storage rejects branch names containing ':' too. 2. restore() read back every .npy file. A name that is not a key put writes (stray.npy, default_garbage.npy, default_2025-3.npy) made delete and get_known_periods raise, so a valid deletion left its value. restore now leaves such files out, with a warning naming them. Tests: examples on both backends and through Simulation for each finding; the memory/disk Hypothesis differential now draws mid-month periods and a ':' branch name and checks both storages accept or reject each put alike; a new Hypothesis property restores a directory of random keys and non-key names and checks restore reads back exactly the keys, warns about the rest, and then deletes like memory. Folds #565's changelog fragment into this PR's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
added a commit
that referenced
this pull request
Oct 7, 2026
Cover the remaining #565 review witness, periods.period(999), on both string-key storage backends. Preserve supported twelve-month aliases and existing derived values after rejected writes. Extend the differential property across the early-year boundary and twelve-unit periods. 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.
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.deletedoes that.OnDiskStorage.delete(period, branch_name)removed only the file keyed exactlyf"{branch_name}_{period}". So a disk-backed simulation (one with aMemoryConfig) kept values the caller had deleted. Here is the issue's reproduction withdelete_arrays("rent", "2025"):[]['2025-01', '2025-02'][][]Change
OnDiskStorage.delete(period, branch_name)now deletes each file key whose parsed branch isbranch_nameand whose periodperiod.contains(...). This is the same rule asInMemoryStorage.delete. Eternal storage still deletes the branch's singleETERNITYkey, anddelete(None, branch_name)keeps #552's whole-branch-name match. As before,deleteonly forgets the mapping and leaves the.npyfile in place.Invariants (tested)
put,delete(period, branch)anddelete(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.delete(p, b), branchbhas no key whose periodpcontains, and every other key is unchanged. Afterdelete(None, b), branchbhas no keys. Both storages are checked against a reference model of this rule after every step._, 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
tests/core/test_disk_storage_delete_contained_periods.pyruns without dev extras. It covers:ETERNITYand multi-unit cases on both backends;Simulation.delete_arraysandHolder.delete_arrayson a disk-backed holder (the OnDiskStorage.delete(period) deletes only that exact period, not the periods within it #564 reproduction).Without the fix, 29 of the 59 fail: every disk case where a period contains another.
tests/core/test_disk_storage_delete_property.pyholds the Hypothesis differential and model test for invariants 1–3. It starts withpytest.importorskip("hypothesis")because the smoke job installs no dev extras. It runs 200 examples in CI; I also ran 5,000 locally with up to 40 steps. Without the fix it shrinks toput(2023, "default")followed bydelete(ETERNITY, "default"): disk keeps 2023.On Windows, the cases whose period string contains
:(multi-unit and rolling-year periods) are skipped. A Windows file name cannot hold that period's key, which predates this PR. Keep branch period reads, dumps, and disk deletion consistent #552's round-trip test now skips them for the same reason (5fe8b61; that was Keep branch period reads, dumps, and disk deletion consistent #552's Windows failure).Full suite on master + Keep branch period reads, dumps, and disk deletion consistent #552 + this PR: 1,735 passed, 4 skipped, 1 xfailed. Country-template YAML: 39 passed.
ruff format --checkandruff checkare clean.Open PRs that touch the same code
Each was merged with this branch on top of master and the full suite run:
on_disk_storage.pycomposes)simulation_dumper.pyconflicts with #552, not this PR. Resolved as #562's body says (#552's branch passed tois_derived)_derived.intersection_updatesits after this PR's comprehension#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_inputsdrops 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_configis set (Holder.__init__). policyengine-us (4e4999a3), policyengine.py (6a9c878), policyengine-canada, -il and -ng never set it, and policyengine-uk (c7e826ea) sets it toNone. 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