Fix countyLookup, which raised for every scope - #172
Open
jinskeep-morpc wants to merge 1 commit into
Open
Conversation
Two bugs, either of which made the class unusable:
- The constructor called DataFrame.from_dict with a columns argument.
pandas accepts columns only for orient="index" or "tight" and rejects it
for the default orient, so instantiating countyLookup raised ValueError
regardless of scope. CONST_COUNTY_NAME_TO_ID maps name -> GEOID, so the
two columns come from its items.
- get_id indexed on a COUNTY_NAME column. The dataframe is filtered to
GEOID and NAME a few lines earlier, so the column does not exist and the
call would have raised KeyError even had the constructor worked.
Both are longstanding. The class had no test coverage, so nothing caught
them; downstream repos worked around it by reading CONST_COUNTY_NAME_TO_ID
directly, and morpc-repotemplate-standardize shipped the broken call to every
repo scaffolded from it.
Add tests covering the scope aliases, the name/GEOID round trip, sort order,
and the three ways a lookup should raise. scope="us" is left uncovered: it
queries the Census API, and get_id is unsupported for it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
morpc.countyLookupcould not be instantiated. Two bugs, either of which alone made the class unusable:__init__calledDataFrame.from_dictwith acolumnsargument. pandas acceptscolumnsonly fororient="index"or"tight"and rejects it for the default orient, so every scope raised.CONST_COUNTY_NAME_TO_IDmaps name → GEOID, so the two columns come from its.items().get_idindexed on aCOUNTY_NAMEcolumn. The dataframe is filtered toGEOIDandNAMEa few lines earlier, so that column does not exist —get_idwould have raisedKeyErroreven if the constructor had worked. This one is invisible until the first bug is fixed, which is presumably why it survived.Both are longstanding rather than a recent regression.
Why it went unnoticed
The class had no test coverage. Downstream, repos worked around it rather than reporting it —
morpc-odrcfacilities-standardizereadsCONST_COUNTY_NAME_TO_IDdirectly with the comment "the class is currently broken against pandas 2.x".morpc-repotemplate-standardizerecommended the broken call in its transform section, so every repo scaffolded from the template inherited a step that fails on a current environment. I have a companion PR removing that recommendation from the template (morpc/morpc-repotemplate-standardize#1); it can stay as-is once this lands, sinceCONST_COUNTY_NAME_TO_IDis the same mapping either way.Tests
10 new tests in
tests/test_morpc.py, covering the scope aliases (oh/ohio/morpc/15-County Region/REGION15/ anyCONST_REGIONSkey), the name ↔ GEOID round trip, sort order and name/id alignment, and the three ways a lookup should raise: unknown scope, unknown county, and a county outside the requested scope.All 10 fail on
mainand pass on this branch.scope="us"is deliberately uncovered — it queries the Census API, andget_idis unsupported for it anyway.Test run
144 passed, 4 failed. The 4 failures are all intests/test_utils.py(ISO 8601 / tzinfo handling indatetime_from_string), fail identically onmain, and are unrelated to this change.🤖 Generated with Claude Code