-
Notifications
You must be signed in to change notification settings - Fork 16
include state FIPS codes in set of all FIPS geo values #1835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
so that `delphi_utils.geomap.GeoMapper().get_geo_values('fips')` also includes (for example) "42000" representing all of Pennsylvania, in addition to FIPS codes in the 42001-42999 interval that represent individual counties. see discussion at cmu-delphi/delphi-epidata#1134
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, tests passing 👍
Also searched the repo for usage of get_geo_values
and found now-redundant code adding megacounties in the validator, so removed those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 👍
@@ -373,13 +373,13 @@ def test_add_geocode(self, geomapper): | |||
def test_get_geos(self, geomapper): | |||
assert geomapper.get_geo_values("nation") == {"us"} | |||
assert geomapper.get_geo_values("hhs") == set(str(i) for i in range(1, 11)) | |||
assert len(geomapper.get_geo_values("fips")) == 3236 | |||
assert len(geomapper.get_geo_values("fips")) == 3293 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 57: 50 states + 1 District (DC) + 6 Territories (Puerto Rico etc)
@@ -166,8 +166,6 @@ def _get_valid_geo_values(self, geo_type): | |||
gmpr = GeoMapper() | |||
valid_geos = gmpr.get_geo_values(geomap_type) | |||
valid_geos |= set(self.params.additional_valid_geo_values.get(geo_type, [])) | |||
if geo_type == "county": | |||
valid_geos |= set(x + "000" for x in gmpr.get_geo_values("state_code")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lolololol, we shoulda just been using _get_valid_geo_values()
instead of the geo_type_translator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha actually, not a bad refactor opportunity!
so that
delphi_utils.geomap.GeoMapper().get_geo_values('fips')
also includes (for example) "42000" representing all of Pennsylvania, in addition to FIPS codes in the 42001-42999 interval that represent individual counties.see background discussion at cmu-delphi/delphi-epidata#1134