-
Notifications
You must be signed in to change notification settings - Fork 594
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
Unused args in VCF merge #5745
Unused args in VCF merge #5745
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5745 +/- ##
===============================================
+ Coverage 87.079% 87.107% +0.028%
- Complexity 31918 31998 +80
===============================================
Files 1942 1943 +1
Lines 146909 147126 +217
Branches 16242 16268 +26
===============================================
+ Hits 127927 128157 +230
+ Misses 13062 13050 -12
+ Partials 5920 5919 -1
|
@jamesemery Would you mind taking a look at this bit of code maintenance? |
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.
Nice finding these unused paths. Looking back it appears that as far back as gatk3 most if this functionally appears to be unused (even in testing). While I would say that some of this functionality seems like it may be useful, the more advanced use cases are already managed by referenceConfidenceVariantContextMerger. Some day we should unify code between the two paths, but this step if anything will help illuminate what is important in such a merge.
Thanks @jamesemery! |
This reverts commit 70d4303.
@lbergelson The merge vcfs code is notoriously hard to understand. Removing a bunch of arguments that the GATK never uses seems like a start.