Repository navigation
Conversation
…D CCS
defined_for masks a formula's output, but policyengine-core runs the formula
over the whole population, so county keys missing from a county-keyed
parameter table raise ParameterNotFoundError in multi-household simulations.
- vt_renter_credit: mask non-VT and UNKNOWN counties to Addison, and cap the
table lookup at household size 8 (new max_household_size parameter), as the
Department of Taxes renter credit calculator does for 9+ person households.
- mn_ccap_provider_rate, wi_shares_{hourly,monthly}_max_rate,
md_ccs_payment_rate: also mask an in-state UNKNOWN county.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each case puts an in-state household, an in-state UNKNOWN-county household and (for the crash guards) an out-of-state household in one simulation, so the defined_for mask is not all false and the formula runs over every county. VT test 12 covers tax units of 8, 9 and 12. All six cases raise ParameterNotFoundError on upstream/main and pass with the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The CI run on this pull request was cancelled at 23:27Z on 2026-10-11 to free the organization's shared GitHub runners for an urgent fix in another repository. The US + core hub will re-run it automatically (cancelled jobs only; jobs that had passed are kept). Nothing in this pull request changed. Please don't push or reopen just to restart CI. |
PR Review: Guard county-keyed lookups in VT renter credit, MN CCAP, WI Shares and MD CCSPR #10116 (author: MaxGhenis, head SummaryThe PR stops five county-keyed parameter lookups from raising
The fixes are right:
One should-fix. The new parameter starts in 2021 but cites only the 2023–2025 calculators. The 2021 and 2022 calculators show the same cap and should be cited. Branch. The branch conflicts with Three suggestions follow. Critical (must fix)None. Should address1.
Suggestions1. Make an
2. A household whose county belongs to another state still raises.
3. Reword the changelog and one PR-body sentence about what crashed on
Verified correct
Axiom line assessmentValid:
CI statusAll 26 checks pass (
Branch status
Verdict: REQUEST_CHANGES |
DTrim99
left a comment
There was a problem hiding this comment.
The guards are right: every county of each state is a table key, in-state results are unchanged, and every new expectation reproduces by hand (the VT values match the department's 2025 calculator). One should item (see the comment above): max_household_size.yaml starts in 2021 but cites only the 2023-2025 calculators. The 2021 and 2022 calculators have the same cap at Calculator!G10-G12, so cite both and fix the cell numbers in the comment, the PR body and rulespec-us#1678. Also, the branch now conflicts with main in vt_renter_credit.py/.yaml (#10130 rounding, #10125 cases). Keep main's whole-dollar base and half-up rounding with this PR's [size][safe_county], and keep all YAML cases.
PR Review (round 2): Guard county-keyed lookups in VT renter credit, MN CCAP, WI Shares and MD CCSPR #10116 (author: MaxGhenis). Head SummaryEvery round-1 item is resolved, and nothing new needs fixing in this PR.
Two suggestions:
Prior findings
New findingsCritical (must fix)None. Should addressNone. Suggestions1. Other county-keyed lookups still have the crash this PR fixes. None of them is new on
2. Put the TY2021–2022 sources into the queued Axiom work itself.
Verified
Axiom line assessmentValid:
CI statusAll 27 checks pass (
Branch status
Verdict: APPROVE |
DTrim99
left a comment
There was a problem hiding this comment.
Round 2: Should 1 (TY2021-22 calculator citations) is fixed, and the new county_has_state_suffix guard handles mismatched-state counties in all five formulas, with paired tests. Out-of-state, UNKNOWN, mismatched and missing counties all fall back, and in-state keys are unchanged. Both hand-resolved merge blocks are correct, and all 20 new expectations hand-check. Two suggestions, including the other lookups still on the old guard (NC SCCA, MD local tax, IN county tax, WA). Full report in the comment above.
Problem
defined_formasks a formula's output, but policyengine-core 3.32.15 runs the formula over the whole population and skips it only when the mask is all false. A county key missing from a county-keyed parameter table therefore raisesParameterNotFoundErrorwhenever the simulation also holds a household the table doesn't cover. This follows #10113 (the WA property tax exemption) and comes from the same 51-state probe.vt_renter_creditmn_ccap_provider_ratewi_shares_hourly_max_rate,wi_shares_monthly_max_ratemd_ccs_payment_rateChanges
where(in_state & county_has_state_suffix(county, "<ST>"), county, "<FALLBACK>_COUNTY_<ST>"). This guards out-of-state rows, UNKNOWN counties and counties belonging to another state. The fallbacks remain Addison (VT), Hennepin (MN), Adams (WI) and Allegany (MD). VT switches from thecountyenum tocounty_strto match the other guards; the keys are the same.gov.states.vt.tax.income.credits.renter.max_household_size= 8, caps the table lookup. Tax units of 9 or more use the 8-person income limits and fair market rent.Why 8 for Vermont
MATCH(TEXT(IF(Household_Size>8,8,Household_Size),0),...).IF(Household_Size>4,4,Household_Size). PE'sfair_market_renttable already repeats the 4-person value through size 8, so capping at 8 is exact.Response
UTF-8 review follow-up: all five guards use the shared
county_has_state_suffixhelper to compare text or byte suffixes without ASCII decoding, preserving county keys and fallbacks; five new!!binaryYAML cases pair a covered household with UTF-8DOÑA_ANA_COUNTY_NMand failed at68051002b4withUnicodeDecodeError. All five regressions now pass; the seven requested files pass 635 tests (VT YAML 38, MN 15, WI hourly 11, WI monthly 8, MD 11, VT amounts 12, VT income limits 540), one foreground process per file with the requested thread caps; Ruff 0.15.5 format/check,make format, andgit diff --checkpass.Tests
New vectorized YAML cases put an in-state household, an in-state UNKNOWN-county household and (for the crash guards) an out-of-state household in one simulation, so the mask is not all false:
vt_renter_credit.yamltest 11: Chittenden, New Hampshire, and VT UNKNOWN (falls back to Addison).vt_renter_credit.yamltest 12: VT tax units of 8, 9 and 12.mn_ccap_provider_rate.yamlCase 13,wi_shares_hourly_max_rate.yamlCase 9,wi_shares_monthly_max_rate.yamlCase 6, and the newmd_ccs_payment_rate.yamlinformal-provider case.Review follow-up cases. One additional case per variable pairs a normal in-state household with an in-state household whose county belongs to another state:
GRAFTON_COUNTY_NHuses Addison:(60,390 - 44,145) × 1,692 / (60,390 - 27,900) = 846; the Chittenden control gives 1,162.MILWAUKEE_COUNTY_WIuses Hennepin:250 × 52 / 12 = 1,083.33; the Becker control uses205 × 52 / 12 = 888.33.HENNEPIN_COUNTY_MNuses Adams' licensed-group table rate of 8.75/hour; the Milwaukee control is 9.72/hour.HENNEPIN_COUNTY_MNuses Adams:ceil(30 × 4.348125) = 131hours,8.75 × 131 = 1,146.25; the Milwaukee control is9.72 × 131 = 1,273.32.FAIRFAX_COUNTY_VAuses Allegany: informal REGULAR UNIT_183 / 3 = 27.67; the Montgomery control is177 / 3 = 59.Original guard validation (before the main merge). With the five formulas restored from
upstream/mainand the new tests kept, the six new cases fail withParameterNotFoundError(keys…thirty_percent.9, the out-of-state VT key,…weekly.CHILD_CARE_CENTER.UNKNOWN,…licensed_group.hourly.UNKNOWN×2,…informal.rates.UNKNOWN). The 44 existing cases in those files pass. With the fix, all 50 pass.Broader runs. All VT, MN, WI and MD baseline YAML tests plus
household/state_property_tax_creditspass (1,920).code_health/test_parameter_yaml_keys.py,code_health/test_reference_urls.pyandtest_md_ccs_service_unit_enum_default.pypass (69).51-state probe (2026). Five multi-household simulations: 51 states with real counties; the same plus UNKNOWN in every state; plus VT tax units of 9 and 12; a lone VT unit of 9; and every county of VT (14), MN (87), WI (72) and MD (24) plus UNKNOWN and out-of-state. Every scenario raised for at least one variable on main; all five variables compute in every scenario here. Script and outputs:
~/reviews/county-lookup-guards-2026-10/evidence/.After the main merge. Main (
35a94c3b02) was merged in ata23810c84e. Main now carries #10130, which rounds the VT credit amount to whole dollars before the phase-out, and the merge resolved the overlap. The PR still touches the same 12 files and the guard and size-cap code is unchanged. Invt_renter_credit.yaml, the worked-arithmetic comments for tests 11 and 12 now show #10130's rounding step; the expected values (1,162, 846, 1,250) are the same. The nine affected test files were rerun one process per file on that exact tree, and all pass:vt_renter_credit.yaml(36),vt_renter_credit_countable_tax_exempt_ss.yaml(2),vt_renter_credit_integration.yaml(2),mn_ccap_provider_rate.yaml(13),wi_shares_hourly_max_rate.yaml(9),wi_shares_monthly_max_rate.yaml(6) andmd_ccs_payment_rate.yaml(9), plus main'stest_vt_renter_credit_amounts.py(12) andtest_vt_renter_credit_income_limits.py(540).Review follow-up validation (HEAD
68051002b4c2d12f6fd6c22e04836b34830af474): All 700 tests passed in the eleven requested files. Each file ran in its own foreground process, sequentially, withOMP_NUM_THREADS=4 OPENBLAS_NUM_THREADS=4 MKL_NUM_THREADS=4 VECLIB_MAXIMUM_THREADS=4 NUMEXPR_NUM_THREADS=4 OMP_WAIT_POLICY=PASSIVE. No new microsimulation was run.vt_renter_credit.yamlmn_ccap_provider_rate.yamlwi_shares_hourly_max_rate.yamlwi_shares_monthly_max_rate.yamlmd_ccs_payment_rate.yamlvt_renter_credit_countable_tax_exempt_ss.yamlvt_renter_credit_integration.yamltest_vt_renter_credit_amounts.pytest_vt_renter_credit_income_limits.pytest_parameter_yaml_keys.pytest_reference_urls.pyRuff 0.15.5:
uvx ruff@0.15.5 format .left all 6,919 Python files unchanged;uvx ruff@0.15.5 check .passed.git diff --checkpassed.Invariants
The probe runs every county of the four states and checks that each variable computes. It does not itself assert main/head equality, size-cap equality, out-of-state zeros or calculator agreement; the YAML cases assert representative values for those:
defined_for).No Python test is added, per the repo's YAML-first rule; the YAML cases cover each path.
Hub microsimulation impact (current head)
A real microsimulation on the default dataset, main at the merge base
35a94c3b02against this PR's head7089303db3, for 2025 and 2026. The runs went one at a time under the shared heavy-job lock. Sums are weighted.state_income_taxincome_taxhousehold_net_incomehousehold_benefitsRun on the standard outputs only. On main, requesting
vt_renter_creditacross the whole population crashes on its out-of-state households (the crash this PR fixes), so a base run that names it cannot complete. No record changes in either year, as at the earlier heads68051002b4,a23810c84eand8fde198e14. A zero is expected: every default-dataset VT, MN, WI and MD household has a county in its own state, so no record reaches a fallback, andvt_renter_creditfeeds the TAXSIM property-credit aggregate, not these four outputs.Comparison files are in
fixes/10116-impact/on the hub host.Hub microsimulation impact (head 6805100)
A real microsimulation on the default dataset, main at the merge base
35a94c3b02against this PR's head68051002b4, for 2025 and 2026. The runs went one at a time under the shared heavy-job lock. Sums are weighted.state_income_taxincome_taxhousehold_net_incomehousehold_benefitsRun on the standard outputs only. On main, requesting
vt_renter_creditacross the whole population crashes on its out-of-state households (the crash this PR fixes), so a base run that names it cannot complete. No record changes in either year, as at the earlier headsa23810c84eand8fde198e14. A zero is expected: every default-dataset VT, MN, WI and MD household has a county in its own state, so no record reaches a fallback, andvt_renter_creditfeeds the TAXSIM property-credit aggregate, not these four outputs.Comparison files are in
fixes/10116-impact/on the hub host.Hub microsimulation impact (merged head a23810c)
A real microsimulation on the default dataset, main at the merge base
35a94c3b02against this PR's heada23810c84e, for 2025 and 2026. The runs went one at a time under the shared heavy-job lock. Sums are weighted.state_income_taxincome_taxhousehold_net_incomehousehold_benefitsRun on the standard outputs only. On main, requesting
vt_renter_creditacross the whole population crashes on its out-of-state households (the crash this PR fixes), so a base run that names it cannot complete. Main already guards out-of-state counties in the MN, WI and MD child-care rate formulas, and every default-dataset household has a real county matching its state, so those three can't reach the UNKNOWN path on this dataset. No record changes in either year, as at the previous head8fde198e14. A zero is expected: no county key changes on this dataset, andvt_renter_creditfeeds the TAXSIM property-credit aggregate, not these four outputs. The independent review found 654 VT tax units in the pinned dataset, the largest with seven people, so the size cap doesn't bind either.Comparison files are in
fixes/10116-impact/on the hub host.Impact
Only cases that crashed on main get new values. In the default dataset every household has a real county matching its state.
vt_renter_credit's only consumer istaxsim_state_property_tax_credit, throughgov.states.household.state_property_tax_credits.axiom: us-vt/statute/32/6066 TheAxiomFoundation/rulespec-us#1678 queued (corpus ingest TheAxiomFoundation/axiom-corpus#816; the rulespec-us corpus pin predates the VT § 6061/§ 6066 rows). The county guards are n/a: pure crash guards.
Follow-up
The TY2025 booklet's Washington County 7-person partial-credit limit ($88,880) differed from the calculator's $88,990. #10125, now on main and in this branch, set it to $88,990, along with seven other VT income-limit corrections.
🤖 Generated with Claude Code