prevent sequence dictionary validation when aligning reads #4308

lbergelson · 2018-01-30T21:48:51Z

previously, tools that align reads required you to manually disable sequence dictionary validation
if you didn't, they would fail because the unaligned bam didn't have the required sequence dictionary

extracting out a SequenceDictionaryValidationArgumentCollection and providing a method for GATKSparkTools to configure it
ReadsPipeline couldn't easily make use of this, so instead it overrides the method that does validation

BwaSpark / BwaAndMarkDuplicatesPipelineSpark now do not require or allow dictionary validation
fixes #4131

codecov-io · 2018-01-30T22:35:37Z

Codecov Report

Merging #4308 into master will increase coverage by 0.017%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #4308       +/-   ##
===============================================
+ Coverage     80.073%   80.091%   +0.017%     
- Complexity     17420     17437       +17     
===============================================
  Files           1080      1081        +1     
  Lines          63131     63201       +70     
  Branches       10200     10215       +15     
===============================================
+ Hits           50551     50618       +67     
  Misses          8587      8587               
- Partials        3993      3996        +3

Impacted Files	Coverage Δ	Complexity Δ
...k/pipelines/BwaAndMarkDuplicatesPipelineSpark.java	`78.947% <100%> (+1.17%)`	`5 <1> (+1)`	⬆️
...org/broadinstitute/hellbender/engine/GATKTool.java	`91.388% <100%> (+0.083%)`	`94 <1> (+1)`	⬆️
...nder/tools/spark/pipelines/ReadsPipelineSpark.java	`89.796% <100%> (+0.665%)`	`14 <2> (+2)`	⬆️
...institute/hellbender/tools/spark/bwa/BwaSpark.java	`77.778% <100%> (+1.307%)`	`7 <1> (+1)`	⬆️
...equenceDictionaryValidationArgumentCollection.java	`100% <100%> (ø)`	`0 <0> (?)`
...stitute/hellbender/engine/spark/GATKSparkTool.java	`83.81% <100%> (+0.314%)`	`57 <6> (+1)`	⬆️
...hellbender/tools/copynumber/GermlineCNVCaller.java	`87.349% <0%> (+0.986%)`	`19% <0%> (+9%)`	⬆️
...utils/smithwaterman/SmithWatermanIntelAligner.java	`80% <0%> (+30%)`	`3% <0%> (+2%)`	⬆️

droazen · 2018-01-30T22:57:06Z

Before I review, can you move the new arg collection out of the spark package and into the usual cmdline.argumentcollections package?

lbergelson · 2018-01-30T23:12:39Z

@droazen Good call.

lbergelson · 2018-05-09T21:07:37Z

@droazen I think this is ready for review

lbergelson · 2018-05-14T19:14:52Z

...e/hellbender/cmdline/argumentcollections/SequenceDictionaryValidationArgumentCollection.java

+import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
+
+/**
+ * interface for argument collections that control


hmn, this trails off

Yes, very non committal...

cmnbroad

A few minor things, plus I think we should make the arg collection overridable in GATKTool as well as GATKSparkTool.

cmnbroad · 2018-05-14T19:20:36Z

src/main/java/org/broadinstitute/hellbender/tools/spark/bwa/BwaSpark.java

@@ -22,8 +23,8 @@
 import java.util.List;

 @DocumentedFeature
-@CommandLineProgramProperties(summary = "Runs BWA",
-        oneLineSummary = "BWA on Spark",
+@CommandLineProgramProperties(summary = "align reads using BWA",


align -> Align

cmnbroad · 2018-05-14T19:21:33Z

src/main/java/org/broadinstitute/hellbender/tools/spark/bwa/BwaSpark.java

-@CommandLineProgramProperties(summary = "Runs BWA",
-        oneLineSummary = "BWA on Spark",
+@CommandLineProgramProperties(summary = "align reads using BWA",
+        oneLineSummary = "align reads to a given reference using BWA on Spark",


align > Align

cmnbroad · 2018-05-14T19:23:10Z

...e/hellbender/cmdline/argumentcollections/SequenceDictionaryValidationArgumentCollection.java

+import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
+
+/**
+ * interface for argument collections that control


Yes, very non committal...

cmnbroad · 2018-05-14T19:25:41Z

...e/hellbender/cmdline/argumentcollections/SequenceDictionaryValidationArgumentCollection.java

+     * most tools will want to use this, it defaults to performing sequence dictionary validation but provides the option
+     * to disable it
+     */
+    class StandardValidationCollection implements SequenceDictionaryValidationArgumentCollection {


I like having these as inner classes so we don't have separate files for them. I assume that its the case that an inner class inside of an interface is static wrt the containing/implementing class ? Can/should these be static ?

They're static by default in an interface and intelli complains that it's redundnat if you mark them as static. I can add it in though if you think it's clearer.

No need to change anything. I just didn't recall seeing that before, but thats about what I suspected.

cmnbroad · 2018-05-14T19:35:01Z

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

-    @Argument(fullName = StandardArgumentDefinitions.DISABLE_SEQUENCE_DICT_VALIDATION_NAME, shortName = StandardArgumentDefinitions.DISABLE_SEQUENCE_DICT_VALIDATION_NAME, doc = "If specified, do not check the sequence dictionaries from our inputs for compatibility. Use at your own risk!", optional = true, common = true)
-    private boolean disableSequenceDictionaryValidation = false;
+    @ArgumentCollection
+    SequenceDictionaryValidationArgumentCollection seqValidationArguments = new SequenceDictionaryValidationArgumentCollection.StandardValidationCollection();


Shouldn't this delegate to a getSequenceDictionaryValidationArgumentCollection() method like the Spark one does ?

Done, although we don't have any use case for it yet.

cmnbroad · 2018-05-14T19:44:46Z

...a/org/broadinstitute/hellbender/tools/spark/pipelines/ReadsPipelineSparkIntegrationTest.java

-                    (knownSites.isEmpty() ? "": " --known-sites " + knownSites) +
-                    " -O %s";
-        }
-


It was totally unused.

cmnbroad · 2018-05-15T16:54:09Z

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

-    @Argument(fullName = StandardArgumentDefinitions.DISABLE_SEQUENCE_DICT_VALIDATION_NAME, shortName = StandardArgumentDefinitions.DISABLE_SEQUENCE_DICT_VALIDATION_NAME, doc = "If specified, do not check the sequence dictionaries from our inputs for compatibility. Use at your own risk!", optional = true, common = true)
-    private boolean disableSequenceDictionaryValidation = false;
+    @ArgumentCollection
+    SequenceDictionaryValidationArgumentCollection seqValidationArguments = getSequenceDictionaryValidationArgumentCollection();


This should be private.

cmnbroad

Noticed one access modifier thing, otherwise 👍 .

previously, tools that align reads required you to manually disable sequence dictionary validation if you didn't, they would fail because the unaligned bam didn't have the required sequence dictionary extracting out a SequenceDictionaryValidationArgumentCollection and providing a method for GATKSparkTools to configure it ReadsPipeline couldn't easily make use of this, so instead it overrides the method that does validation BwaSpark / BwaAndMarkDuplicatesPipelineSpark now do not require or allow dictionary validation fixes #4131

lbergelson · 2018-05-17T20:52:10Z

made it private, will merge when tests pass, thanks @cmnbroad

droazen self-requested a review January 30, 2018 22:06

droazen self-assigned this Jan 30, 2018

lbergelson force-pushed the lb_disable_seqdict_validation_for_bwa branch from ac93a3b to 3195b02 Compare May 9, 2018 21:06

droazen requested review from cmnbroad and removed request for droazen May 14, 2018 19:14

lbergelson commented May 14, 2018

View reviewed changes

droazen assigned cmnbroad and unassigned droazen May 14, 2018

cmnbroad requested changes May 14, 2018

View reviewed changes

lbergelson force-pushed the lb_disable_seqdict_validation_for_bwa branch from 3195b02 to df35942 Compare May 14, 2018 20:11

cmnbroad reviewed May 15, 2018

View reviewed changes

cmnbroad approved these changes May 15, 2018

View reviewed changes

lbergelson force-pushed the lb_disable_seqdict_validation_for_bwa branch from df35942 to 21322db Compare May 17, 2018 20:49

lbergelson assigned lbergelson and unassigned cmnbroad May 17, 2018

lbergelson merged commit ec36b13 into master May 21, 2018

lbergelson deleted the lb_disable_seqdict_validation_for_bwa branch May 21, 2018 20:18

lbergelson mentioned this pull request Jul 11, 2018

BwaSpark does inappropriate validation of bam sequence dictionary against reference #2121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent sequence dictionary validation when aligning reads #4308

prevent sequence dictionary validation when aligning reads #4308

lbergelson commented Jan 30, 2018

codecov-io commented Jan 30, 2018 •

edited

Loading

droazen commented Jan 30, 2018

lbergelson commented Jan 30, 2018

lbergelson commented May 9, 2018

lbergelson May 14, 2018

cmnbroad May 14, 2018

lbergelson May 14, 2018

cmnbroad left a comment •

edited

Loading

cmnbroad May 14, 2018

lbergelson May 14, 2018

cmnbroad May 14, 2018

lbergelson May 14, 2018

cmnbroad May 14, 2018

cmnbroad May 14, 2018

lbergelson May 14, 2018

cmnbroad May 14, 2018

cmnbroad May 14, 2018

lbergelson May 14, 2018

cmnbroad May 14, 2018

lbergelson May 14, 2018

cmnbroad May 15, 2018

cmnbroad left a comment

lbergelson commented May 17, 2018

prevent sequence dictionary validation when aligning reads #4308

prevent sequence dictionary validation when aligning reads #4308

Conversation

lbergelson commented Jan 30, 2018

codecov-io commented Jan 30, 2018 • edited Loading

Codecov Report

droazen commented Jan 30, 2018

lbergelson commented Jan 30, 2018

lbergelson commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad left a comment • edited Loading

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

cmnbroad left a comment

Choose a reason for hiding this comment

lbergelson commented May 17, 2018

codecov-io commented Jan 30, 2018 •

edited

Loading

cmnbroad left a comment •

edited

Loading