get_geo_values() doesnt know about a plain "state" type #1121
Conversation
this is untested!
also has a less hacky feel to it Co-authored-by: dshemetov <[email protected]>
|
this feels much less hacky. added geo_type "county" handling, and it now steps around geo_types that are unknown to GeoMapper (like "dma"). we may want to keep the just-added now this is failing tests, presumably because tests use a geo_type of "fips" when they should specify "county" instead. |
|
I'm working on a test now |
| geo_type_translator = { | ||
| "county": "fips", | ||
| "state": "state_id", | ||
| "zip": "zip", | ||
| "hrr": "hrr", | ||
| "hhs": "hhs", | ||
| "msa": "msa", | ||
| "nation": "nation" | ||
| } | ||
| if geo_type in geo_type_translator: # else geo_type is unknown to GeoMapper |
There was a problem hiding this comment.
praise: nice logic here
agreed long term we should sync with the acquisition types and/or move a shared spec to common but this is a good rapid patch
|
Kudos, SonarCloud Quality Gate passed!
|
|
tests look good. i added a note about syncing geo_type. i think this should be good to go! |
but it does know about "state_id", "state_code", and "state_name" (see delphi_utils/geomap.py). "state_id" appears to be the two-digit state abbreviation, if delphi_utils/data/2020/state_pop.csv is to be believed.
this is untested! i whipped it up fast to address the issue we saw post-rollout. its kinda hacky and i dont love the ternary form of it. ...but it should do the trick, at least in prod for now.
obviously, it should be accompanied by some new additional unit and/or integration tests that involve a "state" geo_type.