-
Notifications
You must be signed in to change notification settings - Fork 82
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
Map where clinical trials are recruiting #569
Conversation
🇫🇷 is missing? |
Yes, that's all you need to do. This GitHub actions step magically takes care of everything else: covid19-review/.github/workflows/update-external-resources.yaml Lines 17 to 23 in e3bd878
|
Per @cgreene's observation, something is very wrong with France (and a few other places) in this geopandas dataset: geopandas/geopandas#1041 |
…ntries because formatting was done on remote
I have zero technical knowledge to help... but looks really cool folks, thank you |
Per @tlukan's suggestion, here is a description for what these figures show (possible draft for a legend): Here is a link to play with the data: http://covid19.trialstracker.net/ |
Currently this is missing the following ISO codes: 'LIE', 'MLT', 'GUF', 'SMR', 'HKG', 'GIB', 'BHR', 'MCO', 'UMI', 'IMN', 'SGP', 'MTQ' |
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.
Just one quick question!
hit = pycountry.countries.search_fuzzy(country + ",") | ||
elif isinstance(hit, type(None)): | ||
hit = pycountry.countries.get(official_name=country) | ||
except LookupError: |
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.
There are two get calls here - does it matter which one raises the lookup error? I'm imagining that search_fuzzy wouldn't raise one form its name.
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 is only the pycountry.countries.get()
command that throws this error, .search_fuzzy()
I believe returns an empty list. Should this section be reformatted to better reflect that? I wasn't quite sure the best way to handle this many levels of possible alternative strategies, although in retrospect I could just make a function that would return a value whenever it first succeeds.
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 noticed there are two .get calls, and if you hit this error the first one you will never do the fuzzy search. We also won't know which .get call failed. I'm not sure that either of these matter, so I wanted to raise them to see if it would make a difference.
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.
Thank you so much for the review @cgreene!
hit = pycountry.countries.search_fuzzy(country + ",") | ||
elif isinstance(hit, type(None)): | ||
hit = pycountry.countries.get(official_name=country) | ||
except LookupError: |
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 is only the pycountry.countries.get()
command that throws this error, .search_fuzzy()
I believe returns an empty list. Should this section be reformatted to better reflect that? I wasn't quite sure the best way to handle this many levels of possible alternative strategies, although in retrospect I could just make a function that would return a value whenever it first succeeds.
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!
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.
This looks good to me. Very cool addition!
@cgreene already approved so I only reviewed the logic lightly. My main suggestion is on how to refer to this figure later in the manuscript.
plt.savefig(args.output_map + '.svg', bbox_inches="tight") | ||
|
||
print(f'Wrote {args.output_map}.png and {args.output_map}.svg') | ||
|
||
# The placeholder will be replaced by the actual SHA-1 hash in separate |
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 suggest moving this block to follow line 208 (the Wrote {args.output_figure}.png and {args.output_figure}.svg
message) to keep the trials figure code grouped together.
Co-authored-by: Anthony Gitter <agitter@users.noreply.github.com>
Thank you both so much for the review! Excited to see if this works overnight :) |
Description of the proposed additions or changes
This is a proposed addition to the EBM Data Lab COVID-19 TrialsTracker analysis that is used to generate some of the existing figures. Here, the countries represented in the EBM dataset are tabulated, with single-country clinical trials and multi-country clinical trials counted separately. Tabulation is done based on ISO codes, which are easier to match than country names.
Here is what it creates:

Questions:
Concerns:
I'm happy to hear suggestions either on the code or visualization side!
Related issues
#552
Suggested reviewers (optional)
@tlukan @agitter @cgreene @rdvelazquez