Repository navigation
Validate integer enum positions before narrowing - #630
Open
juaristi22 wants to merge 2 commits into
Open
juaristi22 wants to merge 2 commits into
juaristi22 wants to merge 2 commits into
Conversation
vahid-ahmadi
approved these changes
Oct 24, 2026
vahid-ahmadi
left a comment
There was a problem hiding this comment.
@juaristi22 thanks, first-round review at 9242c26a. This closes the core half of policyengine-uk#2602 as asked: out-of-range integer positions now fail loudly in Enum.encode, set_input and situation inputs, and valid positions, names and members keep their meaning. No must or should findings.
What I checked
- The issue's core repro. On the base,
Enum.encodereturns an out-of-range integer array unchecked, so it decodes asunknown. At this head it raisesValueError: Invalid position(s) [12] for enum CouncilTaxBand. Integer positions are zero-based and must be in [0, 9).I also checked this end to end: with this branch installed into a policyengine-uk venv,sim.set_input("council_tax_band", 2025, np.array([12]))and a situation withcouncil_tax_band: {2025: 12}both raise that error. With core 3.33.2 (what pe-uk locks today) both silently giveunknown. - The docstring now says integers are zero-based positions in declaration order, not survey source codes, which was point 2 of the issue. The old doctest output (
EnumArray([1, 2, 3])) was wrong too, and is fixed. - Tests in a fresh venv: the two new files give 61 passed. With
policyengine_core/reset to the base037960ca, 28 of the 61 fail and the 33 preservation cases pass, so the tests pin the change. Full suite at head: 2,718 passed, 4 failed, all 4 intests/core/test_release_tagging.py, which fail the same way on the base on my machine (the tag script exits 127 locally), so that is my environment, not this PR. - pe-uk YAML policy tests (
policyengine-core test policyengine_uk/tests/policy) on pe-uk #2627's head with this branch installed: 4,454 passed, so no UK test feeds an out-of-range position today. - Cost:
encodeon an existing 1m-rowEnumArraynow does one bounds pass, about 0.1 ms. Fine for the hot path inholder.py:704. ruff checkandruff format --checkclean on the changed files. CI at head: all 20 checks pass, including the core-plus-country-packages jobs. Mergeable.
Nits, no action needed
- An
EnumArrayalready bound to a different, larger enum is checked against its own enum and returned unchanged (enum.py, theisinstance(array, EnumArray)branch). So a position valid for the owner but out of range for the receiving variable still gets through. The PR body says this is deliberate, it is the existing behaviour, and I couldn't find a real caller that does this, so I'd leave it. - Booleans are let through as integers (
Truereads as position 1). Also existing behaviour; mentioning it only because the new scalar branch skips them on purpose.
Merge order: independent of policyengine-uk#2627, which does its own check at the UK dataset boundary and works on core 3.33.2. The set_input and situation routes in UK only get this fix once core is released and pe-uk's lock moves past 3.33.2, so #2602 should stay open until then, as the body says.
🤖 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.
Refs PolicyEngine/policyengine-uk#2602
Validate integer enum positions before narrowing to the configured enum dtype. Negative and out-of-range positions now raise a ValueError naming the enum and bad values, including encoded arrays and scalar situation inputs. An existing encoded array keeps its identity and attached enum; bounds follow that owner, with the receiving enum used when the array is unbound.
Valid zero-based positions, names and enum members retain their current meanings. Integer positions follow declaration order, not survey source codes; in-range survey codes still need explicit upstream mapping. No enum declarations, policy values or defaults change.
Reproducer:
tests/core/enums/test_integer_enum_bounds.py::test_invalid_integer_positions_raise_contextual_value_erroraxiom: n/a: engineering input validation; no enacted-policy change.
Validation: the affected 167-case selection returned 166 passes, including all 61 new cases, and one unchanged pre-existing expected failure. That inherited ETERNITY test was independently qualified against the base; no marker or allowlist was changed. Two additional regressions reproduced and then verified a review fix for foreign arrays with larger and smaller attached enums. Formatting and lint passed, preserving Python ASTs and all other source bytes. Full Core suite and current-head CI were not run locally; independent exact-commit review is recorded before publication.
This is the Core companion to the UK dataset-boundary fix tracked in #2602. UK remains locked to Core 3.33.2; Core release and UK dependency adoption are separate work. The cross-repository issue stays open for adoption and explicit survey-code mapping where needed.