Fix for NPE when GDB has too many alleles #7738
Conversation
allow variant to pass QUAL filter
|
The NPE was happening at highly multiallelic sites that ALSO had enough GQ0 homozygous reference genotypes to introduce enough doubt about the site being reference that the site passed the QUAL filter. Then it went on to annotation, which is where the NPE arose. I also added a check in the annotation code so we can be more graceful and informative. |
There was a problem hiding this comment.
@ldgauthier Looks good to me -- I've just requested some minor tweaks, then this can go in once tests pass
| @@ -306,6 +309,10 @@ else if ( clazz.equals(int[].class) ) { | |||
| } | |||
|
|
|||
| final double[] likelihoodSums = calculateLikelihoodSums(vc, defaultPloidy, allHomRefData); | |||
| if (MathUtils.sum(likelihoodSums) == 0.0 && !allHomRefData) { | |||
| throw new IllegalStateException("No likelihood sum exceeded zero -- method was called for variant data " + | |||
| "with no variant information."); | |||
There was a problem hiding this comment.
Are users likely to encounter this error in practice? If yes, maybe add a bit more contextual info to make it less cryptic (such as the fact that we're doing allele subsetting and calculating the most likely alleles)
There was a problem hiding this comment.
No, this would be if a developer called this method under the wrong circumstances. In the buggy behavior we spent a loooooong time calculating on all-zero data, which is weird and annoying and this would definitely put a stop to it.
src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypingEngine.java
Outdated
Show resolved
Hide resolved
| @@ -124,11 +124,14 @@ public AFCalculationResult calculate(final VariantContext vc) { | |||
| * Compute the probability of the alleles segregating given the genotype likelihoods of the samples in vc | |||
| * | |||
| * @param vc the VariantContext holding the alleles and sample information. The VariantContext | |||
| * must have at least 1 alternative allele | |||
| * must have at least 1 alternative allele. Hom-ref genotype likelihoods can be approximated | |||
| * but | |||
| * @return result (for programming convenience) | ||
| */ | ||
| public AFCalculationResult calculate(final VariantContext vc, final int defaultPloidy) { | ||
| Utils.nonNull(vc, "VariantContext cannot be null"); | ||
| Utils.validate(vc.getGenotypes().stream().anyMatch(Genotype::hasLikelihoods), | ||
| "VariantContext must contain at least one genotype with likelihoods"); |
There was a problem hiding this comment.
In these error messages, it might be good to include a list of ways that you could end up with genotypes with no likelihoods, to help users debug.
| args.add("-G"); | ||
| args.add("StandardAnnotation"); | ||
| args.add("-G"); | ||
| args.add("AS_StandardAnnotation"); |
There was a problem hiding this comment.
Consider using ArgumentsBuilder in the future
and low quality genotypes allow variant to pass QUAL filter