Adds VcfComparator tool #8933

meganshand · 2024-07-30T20:54:31Z

Tool is useful for WARP testing so that expected differences can be removed from the check.

post-reblocking when many deletion "cleanup" changes occur

ignore

gatk-bot · 2024-07-30T21:15:46Z

Github actions tests reported job failures from actions build 10169605080
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	17.0.6+10	10169605080.12	logs
unit	17.0.6+10	10169605080.1	logs

ldgauthier

The code I gave you to start with is very much gradware that we used to be able to hide in private classes in GATK3, but that era is over. Maybe @droazen will let us create a different walker category to hide it in?

ldgauthier · 2024-07-31T13:36:06Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/GenotypeGVCFsEngine.java

@@ -469,6 +469,13 @@ static List<Genotype> cleanupGenotypeAnnotations(final VariantContext vc, final
                builder.AD(AD);
            }

+            //convert GQ0s that were reblocked back to no-calls for better AN and InbreedingCoeff annotations


I feel like there shouldn't be any edits in this class on this branch if the rebase went properly.

ldgauthier · 2024-07-31T13:37:27Z

src/main/java/org/broadinstitute/hellbender/engine/MultiVariantWalkerGroupedByOverlap.java

+ *
+ * Client tools must implement apply(List<VariantContext> variantContexts, ReferenceContext referenceContext)
+ */
+public abstract class MultiVariantWalkerGroupedByOverlap extends MultiVariantWalker {


I'm not gonna lie, I have no memory of where this class comes from. Apparently this was two years ago, so maybe it shouldn't be a surprise. I think it's a modified version of MultiVariantWalkerGroupedOnStart. I suspect we needed it because I wanted the star alleles to be returned with their deletions? I just talked myself into keeping this, although we probably don't need it for this exact implementation if you turned off the spandel checks.

ldgauthier · 2024-07-31T13:38:16Z

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

@@ -83,6 +85,16 @@ public static boolean isAlleleSpecific(final VariantAnnotation annotation) {
        return annotation instanceof AlleleSpecificAnnotation;
    }

+    /**


I think this can be refactored to use the method below: isAlleleSpecificGatkKey

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/ReblockGVCF.java

ldgauthier · 2024-07-31T13:40:40Z

...a/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparatorIntegrationTest.java

+    }
+
+    @Test(dataProvider = "getTestFiles")
+    public void testAnnotationDifferences(String args, String expectedWarnings) throws IOException {


Can you add a test that puts a huge delta on the VQSLOD (such that -4.8149 and -3.2178 from the existing test are acceptable)?

I don't see an argument to adjust that threshold for VQSLOD currently (am I just missing it?). I see tolerances for other attributes though. Should I add one for VQSLOD and AS_VQSLOD?

I guess if that arg isn't there then we don't need to test it. :-/ I mostly want to demonstrate usage of the inexact match that's already there. I guess you could manually edit a QUAL score.

gotcha, I added a QUAL test

ldgauthier · 2024-07-31T14:44:43Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparator.java

+        }
+    }
+
+    private void validateSingleSampleDeletions(final VariantContext vc, final VariantContext match,


This is specific to a particular GATK version update, but I can't remember which one.

I would imagine that future updates will need adjustments to this tool to account for expected differences. Are we hoping that those changes get committed into master each time? Or is it ok to leave in some of these examples for future users to model their own after?

I'm fine leaving old stuff, but it would probably be good to commit whatever we end up using for WARP test comparisons

That makes sense. I'll add a note about this to the "how to update gatk in warp" doc.

ldgauthier · 2024-07-31T14:46:14Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparator.java

+            throw new UserException("Alleles are mismatched at " + actual.getContig() + ":" + actual.getStart() + ": actual has "
+                    + actual.getAlternateAlleles() + " and expected has " + expected.getAlternateAlleles());
+
+        } else if (allowNewStars && hasNewStar(actual, expected)) {


I wonder if this should be !allowNewStars...

this is hard to write a test for without some example input, but I agree with you so I changed it here. If you know of an example VCF with some changed star alleles let me know.

ldgauthier · 2024-07-31T14:47:11Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparator.java

+                    continue;
+                }
+                final Object actualValue = actual.get(key);
+                if (expectedValue instanceof List && actualValue instanceof List || key.equals(GATKVCFConstants.AS_SB_TABLE_KEY)) { //|| (key.contains("RAW") joint in OR with AS_SB_TABLE


I don't remember what this comment means -- we can probably remove it

ldgauthier · 2024-07-31T14:47:52Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparator.java

+                    for (int i = 0; i < iterationEnd; i++) {
+                        if (i >= actualList.size() || i >= expectedList.size()) {
+                            //TODO: the toStrings here don't do a great job
+                            //throw makeVariantExceptionFromDifference(key, actualValue.toString(), expectedValue.toString());


I feel like this should throw, but maybe I commented it out for testing for some reason...

done (for now at least)

ldgauthier · 2024-07-31T14:53:37Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparator.java

+            } else {
+                alleleBalance = ad[1] / ((double) ad[0] + ad[1]);
+            }
+            if (alleleBalance >= 0.2 && alleleBalance <= 0.8) {  //TODO: this isn't super robust


I don't know why this TODO. This is what the gnomAD criteria state.

droazen · 2024-07-31T18:15:39Z

@ldgauthier Well, there is always the @ExperimentalFeature tag if you think it's appropriate in this case. But if @meganshand did a pass to clean up / refactor the old code, @BetaFeature might be the right label....

meganshand · 2024-08-02T16:54:51Z

@ldgauthier back to you!

ldgauthier

I'm fine leaving this as an ExperimentalFeature with no refactor. I worry a little about there being logic in there to ignore certain differences by default. If you could just do a quick skim with that in mind the I think this is good enough to merge for now.

ldgauthier · 2024-08-06T13:42:59Z

...a/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparatorIntegrationTest.java

+    }
+
+    @Test(dataProvider = "getTestFiles")
+    public void testAnnotationDifferences(String args, String expectedWarnings) throws IOException {


I guess if that arg isn't there then we don't need to test it. :-/ I mostly want to demonstrate usage of the inexact match that's already there. I guess you could manually edit a QUAL score.

ldgauthier · 2024-08-06T13:49:52Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparator.java

+        }
+    }
+
+    private void validateSingleSampleDeletions(final VariantContext vc, final VariantContext match,


I'm fine leaving old stuff, but it would probably be good to commit whatever we end up using for WARP test comparisons

meganshand

I checked in WARP and everywhere that we call this tool we also check that the VCFs are identical with diff (after removing the header). So I think it's ok if we're skipping some differences by default with this tool since the raw diff will catch them. I also skimmed the tool to see what we're skipping and other than some of the defaults for QUAL and inbreedingCoeff, I think most checks are fairly robust.

meganshand · 2024-08-06T17:31:00Z

...a/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparatorIntegrationTest.java

+    }
+
+    @Test(dataProvider = "getTestFiles")
+    public void testAnnotationDifferences(String args, String expectedWarnings) throws IOException {


gotcha, I added a QUAL test

meganshand · 2024-08-06T17:31:53Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/VCFComparator.java

+        }
+    }
+
+    private void validateSingleSampleDeletions(final VariantContext vc, final VariantContext match,


That makes sense. I'll add a note about this to the "how to update gatk in warp" doc.

ldgauthier and others added 14 commits July 30, 2024 09:34

Systematically compare the outputs of two pipeline versions, especially

49c5c83

post-reblocking when many deletion "cleanup" changes occur

Subset annotations to account for different alleles

ddb6ea7

Refactor and clean up

a8236e5

Check ref blocks

ce06629

Be explicit about AS_RAW annotation differences

1f4a335

Update changed star attributes from NaNs to missing; add option to

fea7be4

ignore

Ignoring star attributes still needs work

5efa74a

Lookin' pretty good...

82d5e53

Add new mode to log all differences before failing

9982fff

Misplaced brace prevented PL checks

db49952

Check on */* genotypes was failing DRAGEN outputs -- removed

6a6872f

clean up after rebase

45825a9

adding test

923d1d5

style

2b041df

not sure why we need to cap values for the comparator so removing it

684f021

ldgauthier reviewed Jul 31, 2024

View reviewed changes

addressing comments

9f0310c

ldgauthier approved these changes Aug 6, 2024

View reviewed changes

meganshand commented Aug 6, 2024

View reviewed changes

more comments

6618ca5

meganshand merged commit 096be07 into master Aug 6, 2024
20 checks passed

meganshand deleted the ms_ldg_VCFcomparator branch August 6, 2024 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds VcfComparator tool #8933

Adds VcfComparator tool #8933

meganshand commented Jul 30, 2024

gatk-bot commented Jul 30, 2024 •

edited

Loading

ldgauthier left a comment

ldgauthier Jul 31, 2024

meganshand Jul 31, 2024

ldgauthier Jul 31, 2024

ldgauthier Jul 31, 2024

meganshand Jul 31, 2024

ldgauthier Jul 31, 2024

meganshand Aug 2, 2024

ldgauthier Aug 6, 2024

meganshand Aug 6, 2024

ldgauthier Jul 31, 2024

meganshand Aug 1, 2024

ldgauthier Aug 6, 2024

meganshand Aug 6, 2024

ldgauthier Jul 31, 2024

meganshand Aug 1, 2024

ldgauthier Jul 31, 2024

meganshand Aug 1, 2024

ldgauthier Jul 31, 2024

meganshand Aug 1, 2024

ldgauthier Jul 31, 2024

meganshand Aug 2, 2024

droazen commented Jul 31, 2024

meganshand commented Aug 2, 2024

ldgauthier left a comment

ldgauthier Aug 6, 2024

ldgauthier Aug 6, 2024

meganshand left a comment

meganshand Aug 6, 2024

meganshand Aug 6, 2024

Adds VcfComparator tool #8933

Adds VcfComparator tool #8933

Conversation

meganshand commented Jul 30, 2024

gatk-bot commented Jul 30, 2024 • edited Loading

ldgauthier 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

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

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

Choose a reason for hiding this comment

droazen commented Jul 31, 2024

meganshand commented Aug 2, 2024

ldgauthier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meganshand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatk-bot commented Jul 30, 2024 •

edited

Loading