Adds QD and AS_QD emission from VariantAnnotator on GVS input #8978
Conversation
There was a problem hiding this comment.
One comment about an exception that should/shouldn't be there and some minor comments about documentation but otherwise this looks reasonable to me. It looks like you are just bringing the QD annotation into parity with what it was already doing for AS_QD and it seems reasonable to me that it would operate this way
| } else if (vc.hasAttribute(GATKVCFConstants.RAW_QUAL_APPROX_KEY)) { | ||
| qual = vc.getAttributeAsInt(GATKVCFConstants.RAW_QUAL_APPROX_KEY, 0); | ||
| } else { | ||
| throw new GATKException("No QUAL or QUAL_approx found in when calculating QD at " + vc.getContig() + ":" + vc.getStart()); |
There was a problem hiding this comment.
This exception is a little... messy? It really should never happen based on that check at the top but if it does happen its telling the user something that isn't quite an exception no? If those aren't there we just let the annotation through without issue at the top of this method so why crash here?
There was a problem hiding this comment.
Yeah good point, not sure what I was trying to protect from here.
| if (vc.hasLog10PError()) { | ||
| qual = -10.0 * vc.getLog10PError(); | ||
| } else if (vc.hasAttribute(GATKVCFConstants.RAW_QUAL_APPROX_KEY)) { | ||
| qual = vc.getAttributeAsInt(GATKVCFConstants.RAW_QUAL_APPROX_KEY, 0); |
There was a problem hiding this comment.
can you add a catch for the NumberFormatException here that points out " + vc.getContig() + ":" + vc.getStart()) to the user for later?
| @@ -364,6 +361,42 @@ public void testDeNovo() { | |||
| Assert.assertEquals(lociWithHighConfidenceDeNovo, new int[] {10130767, 10197999}); | |||
| } | |||
|
|
|||
| @Test | |||
| public void addQdToGvsVcf() { | |||
| final File inputVCF = getTestFile("gvsOutput.vcf"); | |||
There was a problem hiding this comment.
Can you rename this to something that gives a little more info about where this is form/why its in the test suite?
| @@ -78,7 +76,9 @@ public List<VCFCompoundHeaderLine> getRawDescriptions() { | |||
| public Map<String, Object> annotate(final ReferenceContext ref, | |||
| final VariantContext vc, | |||
| final AlleleLikelihoods<GATKRead, Allele> likelihoods ) { | |||
| return Collections.emptyMap(); | |||
| // first vc is used for the annotation and the second vc here is used just to get the alleles, so in this case we can pass the same vc for both | |||
| Map<String, Object> annotation = finalizeRawData(vc, vc); | |||
There was a problem hiding this comment.
wait so was this just totally broken except in combineGVCFs?
There was a problem hiding this comment.
I think so, but I'm worried that I'm missing something and this was left unimplemented intentionally for some reason. But the behavior from VariantAnnotator was undesirable in that if you asked for AS_QD it just wouldn't do anything.
| @@ -364,6 +361,42 @@ public void testDeNovo() { | |||
| Assert.assertEquals(lociWithHighConfidenceDeNovo, new int[] {10130767, 10197999}); | |||
| } | |||
|
|
|||
| @Test | |||
| public void addQdToGvsVcf() { | |||
There was a problem hiding this comment.
add a comment explaining that this has QUALapprox but no QUAL and that is what this is testing
Previously, AS_QD was ignored by VariantAnnotator since the
annotate()function always returned an empty map. Now AS_QD will be added.Also QD could only use QUAL as part of the calculation, but GVS output only contains the
QUALapproxannotation. This change allows QD to be calculated fromQUALapproxif QUAL is not present.