Repository navigation
Conversation
…er counts restore_simulation took a group's count as one past its highest member index. With no persons that was max() of an empty array, which raised; with groups that have no members last, it gave too small a count, and restoring their values then raised. It also took the number of persons from a group, so a system with no group entities raised UnboundLocalError. dump_simulation now writes each population's count (count.npy next to id.npy), and restore_simulation uses it. A dump without it gives the smallest count its ids and memberships allow. For a group, that differs from the old count only when it has more ids than one past its highest member index (its last groups have no members); for persons, only when the number of person ids or another group's memberships exceeds the last group's memberships. build_default_simulation used np.arange(count) after #590, which accepts 2.5, 2.0 and other non-integers; the simulation then failed in sum or calculate. It now applies operator.index first, which refuses exactly what range(count) refused before #590, and keeps int64 ids for every integer type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of 619c577: np.save(population.count) saved the count object as given. A masked-array count made the dump raise NotImplementedError, and an integer-like object with only __index__ wrote an object array that restoring refuses (allow_pickle=False). The dump now writes np.int64(operator.index(count)). A count that is not an integer, which only code that sets it by hand can make, gets no count file, so restoring infers it as for an older dump, and the dump does not raise where it used to work. The round-trip property also compares each value's array type, shape and enum class, and checks that every count file is int64. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
marked this pull request as ready for review
October 19, 2026 13:10
Contributor
Author
|
Hand-off to the US + core hub from session
🤖 Generated with Claude Code |
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.
Follow-up to #590. Its reviews found two neighbouring defects outside its scope. This PR fixes both. It leaves #590's behaviour for integer counts unchanged.
What this fixes
1. Restoring a dump with no persons, or with groups that have no members
restore_simulationset each group's count to one past its highest member index, and the number of persons to the length of a group's memberships.max()of an empty array:ValueError: max() iterable argument is empty. This predates Reduce and rank over a population with no persons #590.ValueError: Unable to set value ... as its length is 5 while there are 4 households. Empty groups first or in between were restored correctly.UnboundLocalError.dump_simulationnow writes each population's count, as an int64 scalar, to__entities__/<entity>/count.npy.restore_simulationrestores it.A population with no
count.npygets the smallest count its ids and memberships allow:That covers two cases:
Older Core versions load only the files they know by name, so they still restore new dumps as before.
2. Non-integer counts in
build_default_simulationSince #590,
build_default_simulationusesnp.arange(count, dtype=np.int64). That accepts 2.5, 2.0,np.float64(2.0),np.float32, 0-d float arrays,np.True_,DecimalandFraction. The simulation then fails later:sumraisessize = 2 != 2.5 = count;set_inputworks, butsumandcalculateraiseTypeError.Before #590,
range(count)raisedTypeErrorat build. The method now callsoperator.index(count)first. That refuses exactly whatrangerefuses, becauserangealso converts its argument with__index__.For Python and NumPy integers and 0-d integer arrays,
population.countand the int64 ids are unchanged from #590. An object that is an integer only through__index__builds again, as it did before #590;np.arangerefused it.set_inputthen refuses every array for such a count, because holders compare an array's length with the count.No caller in the repo passes a float;
_get_person_countreturnslen(...)or 1.Invariants
These are the properties the code is meant to hold. The property tests check them by sampling, over the domain they name.
Round trip. Take a simulation in which:
Dump it, then restore it. Then:
This includes zero persons, zero groups, and groups with no members first, last or in between. Every count file is int64.
A count that is not an integer can only be set by hand, and the dump writes none for it. Restoring infers it under the next rule. So it comes back as the integer the ids or memberships give, which can differ in behaviour: with a hand-set
2.0,sumraises before the dump and works after. With no ids and an empty last group, the inferred count is too small and restoring that group's values raises, as at master.test_dump_restore_property.pysamples it on the country template:Dumps without counts. With
count.npyremoved, the round trip still holds when the ids give the counts (one per instance, asSimulationBuildersets them) or the last group has members. Otherwise only the count file gives the count. That case is excluded from the property byassumeand tested as intended intest_dump_keeps_a_count_its_ids_and_memberships_do_not_give.Counts.
build_default_simulation(count=c)raisesTypeErrorexactly whenrange(c)does, and then leaves the simulation it was given untouched. Otherwise, every population's ids and every group's memberships arelist(range(c))as int64, andpopulation.count == c.test_default_simulation_count_property.pysamples it with 300 examples over:None,DecimalandFraction.Tests
Run one file at a time: CPython 3.14.7, NumPy 2.4.2 (
uv.lock), Hypothesis 6.168.3, macOS arm64, 4 BLAS/OpenMP threads.7399c4ed8tests/core/test_dump_restore.py: 11 new testsCOUNT_FILE, which master lacks. 1 (the__index__count) fails at build, because master'snp.arangerefuses it.tests/core/test_dump_restore_property.py(new, 200 examples)COUNT_FILE); with master's restore logic swapped into this branch, it failstests/core/test_default_simulation_count_property.py(new, 300 examples)0.0)tests/core/test_simulation_builder.py: 10 new non-integer cases"2"andNoneraised a differentTypeError, fromnp.arange)tests/core/test_group_population_empty.py: the integer-ID test widened from 4 count types to 14 (every NumPy integer width, signed and unsigned, and 0-d integer arrays)Mutation checks. I applied each of these to the branch and ran all five files. Each fails at least one test:
operator.index;The count property's floats are bounded, so a regression fails in about 1 s instead of allocating
np.arangeof a huge float. An unbounded first version did that at 12:28Z during a mutation check and pushed the host into swap.Full suite at
619c577: every test file undertests/, one at a time. 2,688 passed, 3 xfailed, 1 skipped, 0 failed. The twotests/smokefiles skip locally without the country packages. In CI, the country-package jobs runtest_country_init.py;test_us.py's microsimulation is skipped there too (RUN_SMOKE_TESTS: "0"inpr.yaml). After the review fix, the seven files that dump or restore pass at44e6914:test_dump_restore.py: 12;test_dump_restore_property.py: 1;test_restore_input_registry_property.py: 1;test_restore_input_registry.py: 14;test_restore_input_provenance.py: 2;test_subsample_restore_inputs.py: 8;test_option_result_cache.py: 24.ruff checkandruff format --checkpass.Callers. The local clones of policyengine-us (
2a81a31908), policyengine-uk (d292ae10b) and policyengine.py (dd82d0e59) never calldump_simulation,restore_simulationorbuild_default_simulationdirectly. The independent review found no direct callers in PE-US either.Not run locally. The CI matrix (Python 3.11–3.14 on Ubuntu and Windows) and the country-package jobs; CI runs them.
Review
619c577;~/reviews/us-hub/reviews-gen/core-624/review-r1.md): REQUEST_CHANGES.population.countas given. A masked-array count raisedNotImplementedErroron dump. An__index__-only object wrote an object array, which restore refuses. Fixed in44e6914: the dump writesnp.int64(operator.index(count)), and writes no file for a count that is not an integer.test_dump_writes_any_integer_count_as_int64(uint8, 0-d array, masked array,__index__) andtest_dump_of_a_count_that_is_not_an_integer_infers_it. Four of these fail at619c577and pass now. The 0-d array case already wrote int64 there.COUNT_FILE.44e6914;~/reviews/us-hub/reviews-gen/core-624/review-r2.md): APPROVE, no P1 or P2.619c577(4 failed, 1 passed) and pass here. Mutations of array type, shape, enum class and count-file dtype each fail the property.COUNT_FILEand 1 build;__index__wording and the smoke-test coverage in CI are corrected.Unchanged, noted while testing
scripts/simulation_generator.make_simulationdoes not run on master: it buildsSimulationwithout populations, and it readsis_personon a population rather than its entity. The new test builds that structure (no group ids) by hand.np.array(["h0", "h1"], dtype=object)) can't be restored, at master and here:ValueError: Object arrays cannot be loaded when allow_pickle=False. The builders set numeric or fixed-width string ids.__index__-only count builds again (as before Reduce and rank over a population with no persons #590; master'snp.arangerefuses it), butset_inputthen refuses every array, because holders compare an array's length with the count.range(-1)did.Unsupported array dtype: float64. An empty string or integer array works.axiom: n/a: core infrastructure (dump/restore and default simulation building), no policy change.
🤖 Generated with Claude Code