-
Notifications
You must be signed in to change notification settings - Fork 592
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
M2 doesn't use very short stubs of clipped reads for genotyping #5057
Conversation
@@ -60,6 +60,9 @@ | |||
public static final int MAX_NORMAL_QUAL_SUM = 100; | |||
public static final int MIN_PALINDROME_SIZE = 5; | |||
|
|||
// after trimming to fit the assembly window, throw away read stubs shorter than this length |
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.
Could you add why you do this and cite the issue in github?
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.
done
@davidbenjamin This filter looks fine to me, but could you add a test? You should have the data from the mitochondria bug already, right? Also if this potentially affects HaplotypeCaller should this fix be somewhere deeper so the filter is used by both tools or would we want the fix to be different? |
It probably makes sense to fix HaplotypeCaller as well eventually, but the time scale for changes to HC is much slower and M2 can't afford to wait because, as it turns out, the bug you found doesn't just appear in mitochondria and is hurting sensitivity in the evaluation for our paper. I will write a test. |
Codecov Report
@@ Coverage Diff @@
## master #5057 +/- ##
==============================================
+ Coverage 86.35% 86.402% +0.052%
- Complexity 28824 28895 +71
==============================================
Files 1791 1791
Lines 133601 133789 +188
Branches 14920 14942 +22
==============================================
+ Hits 115364 115596 +232
+ Misses 12834 12794 -40
+ Partials 5403 5399 -4
|
Back to @meganshand. I put in a simple mitochondrial integration test. Given that our MC3 validation already covers this particular bug I actually don't think it needs a new test for mitochondria. Also, for later, are any of your spike-in bams public (or rather, public + public)? I noticed that the NA12878 truth doesn't have very low AFs. |
@davidbenjamin Thanks for the test! Unfortunately none of the spike-in bams I have are public, but I will ask Sarah Calvo if she knows of any samples that would work as a spike-in and are public. Maybe we can track one down. |
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.
👍
Closes #5060.
@meganshand This fixes your bug. Do you have time to review before Monday's release?
@takutosato @LeeTL1220 It improves sensitivity and specificity.
@ldgauthier This probably affects HaplotypeCaller as well.