-
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
fix edge case for Mutect2 germline resource #5563
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5563 +/- ##
===============================================
- Coverage 87.081% 87.079% -0.002%
Complexity 31478 31478
===============================================
Files 1929 1929
Lines 145041 145043 +2
Branches 16074 16075 +1
===============================================
- Hits 126303 126302 -1
+ Misses 12891 12890 -1
- Partials 5847 5851 +4
|
Awesome, glad this is patched up so fast. Will this get picked up by the nightly docker build? I can test tomorrow if so. |
Thank you! I also wonder if you could advise timeline for the new release with this fix. |
The next release is going to be a big 4.1 minor version release, so there are a lot of new features we're trying to finish up. Hopefully it will be by the end of the month, but a nightly will be available with this change by tomorrow: https://hub.docker.com/r/broadinstitute/gatk-nightly |
Just wanted to mention, I'm able to get this error when I use --af-of-alleles-not-in-resource 0.00003125. Noticed the edge case just mentioned 0, but if this hasn't already been dealt with in this fix, I can provide more information. Edit: I've noticed that this happens when I supply -I tumor.bam -I normal.bam -tumor SAMPLE1 and I don't specify a -normal SAMPLE2. If I do include the -normal SAMPLE2, it works fine without error. Not sure if intended or not. |
@pjongeneel Thank you for mentioning this. This PR will also fix the issue when the germline resource has a population allele frequency of 0 or 1 regardless of the default. I believe the problem is coming from using gnomAD vcfs lifted-over to hg38. We have only been using the hg37 gnomAD in development, but it seems that we were naive about the pitfalls of the lift-over. |
@ldgauthier the nightly docker build you pointed to seems behind 4.0.12.0. The Mutect2 error fixed in #5442 is present in the nightly docker build, so I can't test this fix. |
@davidbenjamin Hmm, it doesn't seem to resolve the issue for me. I tested as below and same exception was thrown. Could you confirm if I tested correctly?
$ cat test-unfiltered.vcf
|
@byoo It turns out that the first fix assumed a numerical precision (for comparing the population AF with 1) that isn't guaranteed on every JVM. I just merged another PR (#5560) that checks for If it doesn't work for you, please let me know again and I will do my best to fix it, this time with a very red face!! |
@MikeWLloyd I realized after I posted that link that the nightly Docker build has been broken for some time. We're going to dig into it. |
@davidbenjamin Thank you for your kind explanations! It motivated me to read JVM spec. Variant (also wonder how 0 values for DP and AD can be interpreted) :
Stacktrace:
|
@byoo Thanks, we will fix that one ASAP. By the way, we are doing a big refactoring of Mutect2 filtering, which won't change the outputs but will improve the internal software engineering so that we can write better unit tests to catch these sorts of things. |
@ldgauthier thanks for looking into that as well. I see there is an open issue #4965 that has been revived. I will follow that moving forward. |
PR #5578 should resolve this. |
Thank you for the quick help, @davidbenjamin! By the way, is the phased zero-depth variant called due to #5434? Or, is it an intended call? |
@byoo Given that the TLOD is 16, Mutect2's somatic genotyping is doing what it is supposed to do. However, it makes me wonder if something strange is happening in the assembly. If you want to post a few IGV screenshots of the bamout for this call, sorted by base and including both variant and non-variant tumor reads, as well as the assembled haplotypes, I will try to give an educated guess. |
@davidbenjamin, thank you. I'd like to better interpret those. I am not sure if this is enough. If helpful, I will share the bam file. |
Closes #5553.
@takutosato This is a bug fix for the case
--default-af 0.0
.