Fixes dropped AS_QD bug #6168

ldgauthier · 2019-09-18T14:38:21Z

Addresses #6157

(Very minor tests are failing, which I will fix ASAP.)

genotypes

doesn't have raw annotations; anyway it should get AS_QD

droazen · 2019-09-20T13:50:18Z

src/test/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFsIntegrationTest.java

+//                //Tests for Allele-Specific Annotations
+//                {new File(ALLELE_SPECIFIC_DIRECTORY, "NA12878.AS.chr20snippet.g.vcf"), getTestFile( "AS_Annotations.gatk3.7_30_ga4f720357.expected.vcf"), Arrays.asList( "-A", "ClippingRankSumTest", "-G", "AS_StandardAnnotation", "-G", "StandardAnnotation"), b37_reference_20_21},
+//                {new File(ALLELE_SPECIFIC_DIRECTORY, "NA12878.AS.chr20snippet.g.vcf"), getTestFile( "AS_Annotations.gatk3.7_30_ga4f720357.expected.vcf"), Arrays.asList( "-A", "ClippingRankSumTest", "-G", "AS_StandardAnnotation", "-G", "StandardAnnotation"), b37_reference_20_21},
+//                {new File(ALLELE_SPECIFIC_DIRECTORY, "NA12878.AS.chr20snippet.g.vcf"), getTestFile( "AS_Annotations.keepRawCombined.expected.vcf"), Arrays.asList( "-A", "ClippingRankSumTest", "-G", "AS_StandardAnnotation", "-G", "StandardAnnotation", "-keep-combined"), b37_reference_20_21},


Before I review this branch @ldgauthier, can you explain why you commented out the majority of the GenotypeGVCFs test cases?

droazen

Added my comments @ldgauthier -- back to you

droazen · 2019-09-23T20:48:13Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/annotator/RMSMappingQuality.java

@@ -94,7 +101,7 @@
        final ReducibleAnnotationData<List<Long>> myData = new ReducibleAnnotationData<>(null);
        calculateRawData(vc, likelihoods, myData);
        final String annotationString = makeRawAnnotationString(vc.getAlleles(), myData.getAttributeMap());
-        annotations.put(getRawKeyName(), annotationString);
+        annotations.put(getRawKeyNames().get(0), annotationString);


Perhaps you should write a method that searches the raw key names for the index of the key you want instead of assuming it's the first item. At the very least, declare a named constant for 0 and add some comments explaining your assumptions.

Or, here's another thought: if you can only ever have either 1 or 2 raw keys in this brave new world, maybe you should implement named methods for each of the two keys -- something like getPrimaryRawKeyName() and getSecondaryRawKeyName() (or some more meaningful method names), instead of relying on indices. When you want them both together you can still call getRawKeyNames().

src/main/java/org/broadinstitute/hellbender/tools/walkers/annotator/VariantAnnotatorEngine.java

src/main/java/org/broadinstitute/hellbender/tools/walkers/genotyper/GenotypingEngine.java

src/test/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFsIntegrationTest.java

src/testUtils/java/org/broadinstitute/hellbender/testutils/VariantContextTestUtils.java

exist but don't have to match

ldgauthier · 2019-09-25T18:59:04Z

@droazen tests pass -- look good enough? (My other PR merged this branch, so this should go in first.)

droazen

Back to @ldgauthier with a few final comments/questions

droazen · 2019-10-01T18:44:07Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/annotator/VariantAnnotatorEngine.java

-                    annotationMap.remove(currentASannotation.getRawKeyName());
+                for (final String rawKey : currentASannotation.getRawKeyNames()) {
+                    //here we're assuming that each annotation combines data corresponding to its primary raw key
+                    //AS_QD relies on two keys but doesn't have a combine operation because either GenomicsDB combines the keys


I added a combine operation to AS_QD in the Evoquer branch (https://github.com/broadinstitute/gatk/pull/6011/files#diff-026071e8dae8a8d22d918a4bf65423da), which will likely be merged fairly soon. Will we run into problems in that branch due to any assumptions being made here?

No, the primary raw key for AS_QUAL is the one you have -- AS_QUALapprox

Having two keys for that one is odd, but I wanted to use a different key when we're approximating rather than when we're using David B.'s full EM. What do you think about using the same key with an additional flag annotation if we're approximating?

Well, if we're allowing multiple raw keys in general, the AS_QUAL/AS_QUALapprox thing seems reasonable to me. Having to consult a separate annotation to determine how to interpret the AS_QUAL seems worse/more cumbersome.

droazen · 2019-10-01T18:57:56Z

...adinstitute/hellbender/tools/walkers/annotator/allelespecific/AS_BaseQualityRankSumTest.java

@@ -43,7 +42,7 @@
    }

    @Override
-    public String getRawKeyName() { return GATKVCFConstants.AS_RAW_BASE_QUAL_RANK_SUM_KEY;}
+    public List<String> getRawKeyNames() { return Arrays.asList(GATKVCFConstants.AS_RAW_BASE_QUAL_RANK_SUM_KEY);}


The indexing scheme you added in the most recent revision is an improvement, but it's a bit brittle, since getRawKeyNames() doesn't directly call into getPrimaryRawKeyIndex() when constructing its list. In theory, someone could modify one method but not the other...

I think you should either:

Come up with a scheme where getRawKeyNames() can call directly into the method(s) defining key order
(best solution, but a little work)

or:

Write a unit test for every single getRawKeyNames() method that asserts that you get the expected keys in the expected order, and also for the getPrimaryRawKeyIndex() methods asserting that you always get 0.

droazen · 2019-10-02T14:21:30Z

...rg/broadinstitute/hellbender/tools/walkers/annotator/allelespecific/ReducibleAnnotation.java

@@ -18,7 +18,8 @@
 *
 */
 public interface ReducibleAnnotation extends Annotation {
-    public abstract String getRawKeyName();
+    public abstract List<String> getRawKeyNames();
+    public abstract int getPrimaryRawKeyIndex();


Add javadoc here explaining these new interface methods.

droazen · 2019-10-02T14:30:46Z

src/testUtils/java/org/broadinstitute/hellbender/testutils/VariantContextTestUtils.java

     * @param actual                Variant context to test for equality
     * @param expected              Expected result
-     * @param attributesToIgnore    Attributes we want to exclude from comparision
+     * @param attributesToIgnore    Attributes we want to exclude from numerical comparision, but ensure they exist
+     * @param attributesWithJitter


I don't think the comment here is right -- your code above ensures that attributesWithJitter exist, but doesn't check the existence of the attributesToIgnore

Address other comments

droazen · 2019-10-03T18:14:09Z

@ldgauthier Travis finally ran, but unfortunately there were multiple test failures :(

ldgauthier · 2019-10-03T20:10:43Z

@droazen Travis is green! Ready to go?

droazen

👍 Latest version looks good, and tests pass. Merging!

You should probably eventually push up a default implementation of getRawKeyNames() into ReducibleAnnotation that calls getPrimaryRawKey() followed by getSecondaryRawKeys() (so that each annotation doesn't have to worry about implementing this method correctly), but this can wait for another day.

ldgauthier added 2 commits September 18, 2019 14:22

Replace dropped AS_QD annotations for calculations based on AS_QUAL from

0a7c7d3

genotypes

Square with David's fix and more comments

9f935c0

ldgauthier force-pushed the ldg_fixAS_QDfromGenotypes branch from 9dd22f2 to 9f935c0 Compare September 18, 2019 18:34

I forget why the OxoG test asks for allele-specific annotations, but

71b8d19

doesn't have raw annotations; anyway it should get AS_QD

ldgauthier requested a review from droazen September 19, 2019 16:44

droazen self-assigned this Sep 20, 2019

droazen reviewed Sep 20, 2019

View reviewed changes

Turn tests back on :-P

e880cc3

droazen suggested changes Sep 23, 2019

View reviewed changes

droazen assigned ldgauthier and unassigned droazen Sep 23, 2019

ldgauthier added 3 commits September 24, 2019 11:08

Fix attribute check

7635ece

Update magic index for annotation raw keys

04e22eb

Rectify attributes that don't have to exist versus attributes that must

9ada0ce

exist but don't have to match

ldgauthier assigned droazen and unassigned ldgauthier Sep 25, 2019

droazen suggested changes Oct 2, 2019

View reviewed changes

ldgauthier added 3 commits October 2, 2019 16:15

More elegant handling of multiple raw keys

70e8197

Address other comments

Typo (also rerun tests)

11cfcd2

Comment

8a116e2

Missed some secondary checks

e82b3f8

droazen approved these changes Oct 3, 2019

View reviewed changes

droazen merged commit 406c69c into master Oct 3, 2019

droazen deleted the ldg_fixAS_QDfromGenotypes branch October 3, 2019 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes dropped AS_QD bug #6168

Fixes dropped AS_QD bug #6168

ldgauthier commented Sep 18, 2019

droazen Sep 20, 2019

droazen left a comment

droazen Sep 23, 2019

ldgauthier commented Sep 25, 2019

droazen left a comment

droazen Oct 1, 2019

ldgauthier Oct 2, 2019

droazen Oct 2, 2019

droazen Oct 1, 2019

droazen Oct 2, 2019

droazen Oct 2, 2019

droazen commented Oct 3, 2019

ldgauthier commented Oct 3, 2019

droazen left a comment

Fixes dropped AS_QD bug #6168

Fixes dropped AS_QD bug #6168

Conversation

ldgauthier commented Sep 18, 2019

Choose a reason for hiding this comment

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldgauthier commented Sep 25, 2019

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen commented Oct 3, 2019

ldgauthier commented Oct 3, 2019

droazen left a comment

Choose a reason for hiding this comment