Changed ValidateVariants to do all possible validations in the defaul…

…t case rather than silently doing none (#5984)
broadinstitute · Jun 17, 2019 · 083aac8 · 083aac8
1 parent 744987b
commit 083aac8
Show file tree

Hide file tree

Showing 2 changed files with 93 additions and 7 deletions.
diff --git a/src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/ValidateVariants.java b/src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/ValidateVariants.java
@@ -209,6 +209,18 @@ public void onTraversalStart() {
             genomeLocSortedSet = new GenomeLocSortedSet(new GenomeLocParser(seqDictionary));
         }
         validationTypes = calculateValidationTypesToApply(excludeTypes);
+
+        //warn user if certain requested validations cannot be done due to lack of arguments
+        if(dbsnp.dbsnp == null && (validationTypes.contains(ValidationType.ALL) || validationTypes.contains(ValidationType.IDS)))
+        {
+            logger.warn("IDS validation cannot be done because no DBSNP file was provided");
+            logger.warn("Other possible validations will still be performed");
+        }
+        if(!hasReference() && (validationTypes.contains(ValidationType.ALL) || validationTypes.contains(ValidationType.REF)))
+        {
+            logger.warn("REF validation cannot be done because no reference file was provided");
+            logger.warn("Other possible validations will still be performed");
+        }
     }
 
     @Override
@@ -304,17 +316,20 @@ private Set<String> getRSIDs(FeatureContext featureContext) {
      * @return the final set of type to validate. May be empty.
      */
     private Collection<ValidationType> calculateValidationTypesToApply(final List<ValidationType> excludeTypes) {
-        if (VALIDATE_GVCF && !excludeTypes.contains(ValidationType.ALLELES)) {
+
+        //creates local, temp list so that original list provided by user doesn't get modified
+        List<ValidationType> excludeTypesTemp = new ArrayList<>(excludeTypes);
+        if (VALIDATE_GVCF && !excludeTypesTemp.contains(ValidationType.ALLELES)) {
             // Note: in a future version allele validation might be OK for GVCFs, if that happens
             // this will be more complicated.
             logger.warn("GVCF format is currently incompatible with allele validation. Not validating Alleles.");
-            excludeTypes.add(ValidationType.ALLELES);
+            excludeTypesTemp.add(ValidationType.ALLELES);
         }
-        if (excludeTypes.isEmpty()) {
+        if (excludeTypesTemp.isEmpty()) {
             return Collections.singleton(ValidationType.ALL);
         }
-        final Set<ValidationType> excludeTypeSet = new LinkedHashSet<>(excludeTypes);
-        if (excludeTypes.size() != excludeTypeSet.size()) {
+        final Set<ValidationType> excludeTypeSet = new LinkedHashSet<>(excludeTypesTemp);
+        if (excludeTypesTemp.size() != excludeTypeSet.size()) {
             logger.warn("found repeat redundant validation types listed using the --validation-type-to-exclude argument");
         }
         if (excludeTypeSet.contains(ValidationType.ALL)) {
@@ -345,8 +360,29 @@ private void applyValidationType(VariantContext vc, Allele reportedRefAllele, Al
         // The workaround is to not pass an empty list.
         switch( t ) {
             case ALL:
-                if (!rsIDs.isEmpty()) {
-                    vc.extraStrictValidation(reportedRefAllele, observedRefAllele, rsIDs);
+                if(hasReference())
+                {
+                    if(!rsIDs.isEmpty())
+                    {
+                        vc.extraStrictValidation(reportedRefAllele, observedRefAllele, rsIDs);
+                    }
+                    else{
+                        vc.validateReferenceBases(reportedRefAllele, observedRefAllele);
+                        vc.validateAlternateAlleles();
+                        vc.validateChromosomeCounts();
+                    }
+                }
+                else{
+                    if (rsIDs.isEmpty())
+                    {
+                        vc.validateAlternateAlleles();
+                        vc.validateChromosomeCounts();
+                    }
+                    else{
+                        vc.validateAlternateAlleles();
+                        vc.validateChromosomeCounts();
+                        vc.validateRSIDs(rsIDs);
+                    }
                 }
                 break;
             case REF:

diff --git a/...est/java/org/broadinstitute/hellbender/tools/walkers/ValidateVariantsIntegrationTest.java b/...est/java/org/broadinstitute/hellbender/tools/walkers/ValidateVariantsIntegrationTest.java
@@ -69,6 +69,55 @@ public void testGoodFile2() throws IOException {
         spec.executeTest("test good file", this);
     }
 
+    //Test 1 - default (no exclusion -> ALL), no ref, no dbsnp
+    @Test
+    public void testBadEverythingDefaultNoRefNoDBNSP() throws IOException {
+        IntegrationTestSpec spec = new IntegrationTestSpec(
+                baseTestStringWithoutReference(false, "validationExampleBad.vcf", false, ALL),
+                0,
+                UserException.FailsStrictValidation.class
+        );
+
+        spec.executeTest("test that validation occurs w/o dbsnp or ref files", this);
+    }
+
+
+    //Test 2 - default, no ref, yes dbsnp
+    @Test
+    public void testBadEverythingDefaultNoRefYesDBSNP() throws IOException {
+        IntegrationTestSpec spec = new IntegrationTestSpec(
+                baseTestStringWithoutReference(false, "validationExampleBad.vcf", false, ALL) + " --dbsnp " + hg19_chr1_1M_dbSNP_modified,
+                0,
+                UserException.FailsStrictValidation.class
+        );
+
+        spec.executeTest("test that validation occurs w/o ref, w/ dbsnp", this);
+    }
+
+    //Test 3 - default, yes ref, no dbsnp
+    @Test
+    public void testBadEverythingDefaultYesRefNoDBSNP() throws IOException {
+        IntegrationTestSpec spec = new IntegrationTestSpec(
+                baseTestString(false, "validationExampleBad.vcf", false, ALL),
+                0,
+                UserException.FailsStrictValidation.class
+        );
+
+        spec.executeTest("test that validation occurs w/ ref, w/o dbsnp", this);
+    }
+
+    //Test 4 - default, yes ref, yes dbsnp
+    @Test
+    public void testBadEverythingDefaultYesRefYesDBSNP() throws IOException {
+        IntegrationTestSpec spec = new IntegrationTestSpec(
+                baseTestString(false, "validationExampleBad.vcf", false, ALL)  + " --dbsnp " + hg19_chr1_1M_dbSNP_modified,
+                0,
+                UserException.FailsStrictValidation.class
+        );
+
+        spec.executeTest("test that validation occurs w/ ref, w/ dbsnp", this);
+    }
+
     @Test
     public void testBadRefBase1() throws IOException {
         IntegrationTestSpec spec = new IntegrationTestSpec(
@@ -124,6 +173,7 @@ public void testBadChrCount2() throws IOException {
         spec.executeTest("test bad chr counts #2", this);
     }
 
+    //note - validationExampleBadRSID.vcf also has faulty CHR_COUNTS
     @Test
     public void testBadID() throws IOException {
         final IntegrationTestSpec spec = new IntegrationTestSpec(