-
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
removed arrays code, renamed packages #7260
Conversation
On second thought -- to simplify the merge to master we may take a different approach (copying files, squashing, etc) so I'm updating the approach above for 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.
one minor suggestion
@@ -178,8 +178,6 @@ public void onTraversalStart() { | |||
case GENOMES: | |||
vetTsvCreator = new VetTsvCreator(sampleIdentifierForOutputFileName, sampleId, tableNumberPrefix, outputDir); | |||
break; | |||
case ARRAYS: |
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.
do we even need this switch statement? we make the same call for both exomes and genomes
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.
Yes! I was restraining myself to clean up code and save that for the next PR. Mostly to make the review of this PR easy (ie it's all structural, you don't need to look at each line of code) and the next one smaller, but where the lines of code matter. We also have some "common" code that is now really tool specific that we can shift. Things like the above, etc.
Conceptually -- two things happen in this PR.
...gvs
|- ingest
|- filtering
|- extract
- common
All tests pass (can be run with
./gradlew test --tests "org.broadinstitute.hellbender.tools.gvs*"
). I will run a full workflow in Terra (import, train, extract) and verifyBEFORE we merge, we'll the existing "ah_var_store" to indicate this is where array code lives