-
Notifications
You must be signed in to change notification settings - Fork 2
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
HDXDSYS-742 global coverage #160
Conversation
Test Results1 tests - 16 0 ✅ - 17 8s ⏱️ - 15m 4s For more details on these errors, see this check. Results for commit 2e11555. ± Comparison against base commit a19efdd. ♻️ This comment has been updated with latest results. |
Pull Request Test Coverage Report for Build 10838185780Details
💛 - Coveralls |
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.
Great work on this Briar!
@@ -70,12 +88,22 @@ def populate(self) -> None: | |||
add_missing_value_message( | |||
errors, identifier, "admin 2 code", adm2_name | |||
) | |||
continue | |||
if adm1_code is None: | |||
# Map units that cannot be p-coded to national level |
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.
Will the units that cannot be p-coded be mapped to UNSPECIFIED admin 2 codes of the form CD41-XXX or COD-XXX-XXX?
Should this change be added to food security (not necessarily in this PR)?
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.
Units that can't be p-coded at adm1 will be mapped to unspecified units like COD-XXX-XXX. If we can p-code them to adm1 but not adm2, then they'll be mapped to units like CD41-XXX.
I think it would be good to add this kind of logic to food security! The more data we can get into HAPI the better, even if we can only p-code it to a certain level.
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.
Thx Briar
self.hapi_countries = countries | ||
else: | ||
self.hapi_countries = list( | ||
Country.countriesdata()["countries"].keys() |
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.
I think this should be added as a method to HDX Python Country at some point
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.
It will be amazing to have this new feature!
Any idea how it impacts the run time?
The only pipeline I'm concerned about is conflict events, which ran ok on my computer. I'll test the database export before releasing! |
Hdxdsys 1024 provider admin names
Incorporated into develop-global branch |
I've added global coverage for all of the standardized data categories. The ones left are non-standard data: operational presence, population, and poverty rate. I left food security out of this branch because I think it's worth waiting to incrementally p-code the data if we can. If we need to p-code it first I'll make a new branch.