-
Notifications
You must be signed in to change notification settings - Fork 587
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
Add python script to our repo #8282
Conversation
f2d6813
to
fe2ad35
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## ah_var_store #8282 +/- ##
================================================
Coverage ? 86.097%
Complexity ? 35607
================================================
Files ? 2197
Lines ? 167119
Branches ? 18007
================================================
Hits ? 143884
Misses ? 16802
Partials ? 6433 |
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, just a few questions about potential renaming
) | ||
|
||
|
||
def import_gvs(refs: 'List[List[str]]', |
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.
Weren't we going to rename this something other than import_gvs at this stage? It's obviously not necessary, but I thought I remember the team discussing that
@@ -24,6 +24,7 @@ | |||
|
|||
def import_gvs(argsfn, vds_path, references_path, temp_path): |
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.
see my comment above about potentially renaming this function from import_gvs to something else. It is not strictly necessary, but if it were to be done this is the ticket that we were going to do it in, I believe
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.
not an expert on this code, but LGTM
The first step of taking this code from the Hail team is just pasting it into our repo
Successful run:
https://job-manager.dsde-prod.broadinstitute.org/jobs/49d62f48-2dee-417c-aa65-411cbe47be17
GvsQuickstartHailIntegration--we remove the whl from the integration test---sure seems like we wont need one going forward!
Another ticket will be made for these next steps:
Likely this will need to end up in our docker image and the WDL that creates the Avro files can make a version of the input for this scripts instead
Next we will want to remove the Tranches calculations and instead of that value passed in as yet another parameter
Phasing and dropping GQ0