Skip to content

get_geo_values() doesnt know about a plain "state" type #1121

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 5, 2023
Merged

get_geo_values() doesnt know about a plain "state" type #1121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26947bb
get_geo_values() doesnt know about a plain "state" type
melange396 Apr 5, 2023
cae510f
expanded to cover "county" geo_type as well
melange396 Apr 5, 2023
4136db4
Add and repair tests supporting covidcast geo types
krivard Apr 5, 2023
923ac4c
added note to keep geo_types in sync
melange396 Apr 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions integrations/client/test_delphi_epidata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ def test_geo_value(self):
# insert placeholder data: three counties, three MSAs
N = 3
rows = [
CovidcastTestRow.make_default_row(geo_type="fips", geo_value=FIPS[i], value=i)
CovidcastTestRow.make_default_row(geo_type="county", geo_value=FIPS[i], value=i)
for i in range(N)
] + [
CovidcastTestRow.make_default_row(geo_type="msa", geo_value=MSA[i], value=i*10)
Expand All @@ -245,7 +245,7 @@ def fetch(geo):
self.assertEqual(request['epidata'], [counties[0]])
# test fetch a specific yet not existing region
request = fetch('55555')
self.assertEqual(request['message'], 'Invalid geo_value(s) 55555 for the requested geo_type fips')
self.assertEqual(request['message'], 'Invalid geo_value(s) 55555 for the requested geo_type county')
# test fetch a multiple regions
request = fetch([FIPS[0], FIPS[1]])
self.assertEqual(request['message'], 'success')
Expand All @@ -256,10 +256,10 @@ def fetch(geo):
self.assertEqual(request['epidata'], [counties[0], counties[2]])
# test fetch a multiple regions but one is not existing
request = fetch([FIPS[0], '55555'])
self.assertEqual(request['message'], 'Invalid geo_value(s) 55555 for the requested geo_type fips')
self.assertEqual(request['message'], 'Invalid geo_value(s) 55555 for the requested geo_type county')
# test fetch a multiple regions but specify no region
request = fetch([])
self.assertEqual(request['message'], 'geo_value is empty for the requested geo_type fips!')
self.assertEqual(request['message'], 'geo_value is empty for the requested geo_type county!')
# test fetch a region with no results
request = fetch([FIPS[3]])
self.assertEqual(request['message'], 'no results')
Expand Down Expand Up @@ -326,7 +326,7 @@ def test_async_epidata(self):
# insert placeholder data: three counties, three MSAs
N = 3
rows = [
CovidcastTestRow.make_default_row(geo_type="fips", geo_value=FIPS[i-1], value=i)
CovidcastTestRow.make_default_row(geo_type="county", geo_value=FIPS[i-1], value=i)
for i in range(N)
] + [
CovidcastTestRow.make_default_row(geo_type="msa", geo_value=MSA[i-1], value=i*10)
Expand Down
19 changes: 15 additions & 4 deletions src/server/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,21 @@ def __init__(self, geo_type: str, geo_values: Union[bool, Sequence[str]]):
if not isinstance(geo_values, bool):
if geo_values == ['']:
raise ValidationFailedException(f"geo_value is empty for the requested geo_type {geo_type}!")
allowed_values = delphi_utils.geomap.GeoMapper().get_geo_values(geo_type)
invalid_values = set(geo_values) - set(allowed_values)
if invalid_values:
raise ValidationFailedException(f"Invalid geo_value(s) {', '.join(invalid_values)} for the requested geo_type {geo_type}")
# TODO: keep this translator in sync with CsvImporter.GEOGRAPHIC_RESOLUTIONS in acquisition/covidcast/ and with GeoMapper
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
Comment on lines +62 to +71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

allowed_values = delphi_utils.geomap.GeoMapper().get_geo_values(geo_type_translator[geo_type])
invalid_values = set(geo_values) - set(allowed_values)
if invalid_values:
raise ValidationFailedException(f"Invalid geo_value(s) {', '.join(invalid_values)} for the requested geo_type {geo_type}")
self.geo_type = geo_type
self.geo_values = geo_values

Expand Down
4 changes: 4 additions & 0 deletions tests/server/test_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ def test_parse_geo_arg(self):
self.assertEqual(parse_geo_arg(), [GeoSet("fips", True)])
with app.test_request_context(f"/?geo=fips:{FIPS[0]}"):
self.assertEqual(parse_geo_arg(), [GeoSet("fips", [FIPS[0]])])
with self.subTest("covidcast"):
for geo_type in "county dma hhs hrr msa nation state".split():
with app.test_request_context(f"/?geo={geo_type}:*"):
self.assertEqual(parse_geo_arg(), [GeoSet(geo_type, True)])
with self.subTest("single list"):
with app.test_request_context(f"/?geo=fips:{FIPS[0]},{FIPS[1]}"):
self.assertEqual(parse_geo_arg(), [GeoSet("fips", [FIPS[0], FIPS[1]])])
Expand Down