Reject unsupported monthly household inputs#340
Conversation
There was a problem hiding this comment.
Well-scoped guardrail that does exactly what the issue asked for. The shared common/household.py is the right home, validation runs before the country-package import, and the tests cover both UK and US for the year check and the periodized-value check on person and group entities.
Minor comments
- UK docstring not updated.
src/policyengine/tax_benefit_models/uk/household.pystill says only "unknown or mis-placed variable names, or unknown reform parameter paths" underRaises:. The US docstring was updated; worth mirroring on UK for parity. - Behavior change for
yearas string. Before this PR,year=\"2026\"flowed through as a string. After, it's normalized toint(2026). Almost certainly an improvement but not called out in the PR description — worth a line. _validate_unperiodized_valuesonly flagsMappingvalues. A list likeemployment_income=[1000, 2000]won't be caught. That's outside the issue's scope (which named periodized dicts specifically), so fine — just flagging in case the guardrail expands later.- Soft coupling in
_input_location. Therecord_count == 1 and entity != \"people\"branch works because callers always pass group entities as[value]and thepeoplekey for the multi-record list. Acceptable since the function is module-private and both call sites are in this PR. - Optional: include the offending year in the error.
_annual_period_error()is constant; addingf\"received year={year!r}\"would make the failure slightly more debuggable.
There was a problem hiding this comment.
One observation on the new approach
Instead of normalizing "2026" → 2026 once at the boundary, this commit widens the type to Union[int, str] and threads it through _build_situation, compile_reform, _reform_dict_to_parameter_values, _compile_reform_to, compile_reform_to_policy, compile_reform_to_dynamic. Functionally fine — f"{year}-01-01" and str(year) work for both — but it does mean every public reform-compilation entry point now exposes the wider type to its own callers.
Either approach is defensible; the verbatim path is more honest about caller intent, the normalize-early path keeps downstream signatures narrower. If you ever want to revisit, the smaller-blast-radius option is to normalize in validate_annual_household_inputs and keep compile_reform's year as int. Not blocking.
Tiny nit
AnnualYear = Union[int, str] is defined in common/household.py but compile_reform declares the same union inline. If the alias is meant to be the canonical name for "a year a household calculator accepts", export it from common/__init__.py and reuse it in reform.py; otherwise drop the alias and inline both. Either way is fine — just inconsistent right now.
LGTM to merge.
|
Thanks for review @vahid-ahmadi. I had actually intended to address your first item in my changes. I will fix that, push, then merge. |
Fixes #339
Summary
This PR adds an immediate shared guardrail to the household calculators while monthly household support remains undesigned/unsupported.
It now fails early when callers pass:
yearvalue such as"2026-01"{"2026-01": 1000}The validation is shared across the US and UK household calculators and uses country-neutral error messages. It preserves supported reform effective-date dictionaries, e.g.
reform={...: {"2026-01-01": value}}.Notes
This is intentionally narrow. It does not implement full monthly household support and does not change the annual flattened HDF5 dataset structure.
Tests
python -m py_compile src/policyengine/tax_benefit_models/common/household.py src/policyengine/tax_benefit_models/us/household.py src/policyengine/tax_benefit_models/uk/household.py tests/test_household_impact.pyuv run pytest tests/test_household_impact.py -quv run pytest tests/test_household_impact.py -q -k "monthly_year_period or periodized_person_input or periodized_group_input"uv run ruff check src/policyengine/tax_benefit_models/common/household.py src/policyengine/tax_benefit_models/common/__init__.py src/policyengine/tax_benefit_models/us/household.py src/policyengine/tax_benefit_models/uk/household.py tests/test_household_impact.pyuv run ruff format --check src/policyengine/tax_benefit_models/common/household.py src/policyengine/tax_benefit_models/common/__init__.py src/policyengine/tax_benefit_models/us/household.py src/policyengine/tax_benefit_models/uk/household.py tests/test_household_impact.py