Added --sites-only argument to GATK engine #4764

jamesemery · 2018-05-11T20:18:13Z

jamesemery · 2018-05-11T20:25:41Z

@ldgauthier This is what you requested correct?

ldgauthier · 2018-05-11T20:27:16Z

@jamesemery Those of us that work with thousands of samples thank you.
If you could change "GT" to "genotype" in the arg description then this can be good to go. (Technically the GT field is part of the genotype field, along with AD, DP and all our other friends. Obviously it would be silly to output those without GT, but I aim for semantic correctness.)

droazen

Review complete, back to @jamesemery. Looks good, minor comments only.

droazen · 2018-05-11T20:48:10Z

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

@@ -113,6 +113,10 @@
            optional = true)
    public boolean disableBamIndexCaching = false;

+    @Argument(fullName = StandardArgumentDefinitions.SITES_ONLY_LONG_NAME,
+            doc = "If true, don't emit GT fields when writing vcf file output.", optional = true)
+    public boolean sitesOnlyMode = false;


Name of field should make it clear that this applies to VCFs specifically -- eg., outputSitesOnlyVCFs instead of sitesOnlyMode

droazen · 2018-05-11T20:50:37Z

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

@@ -728,6 +732,10 @@ public VariantContextWriter createVCFWriter(final File outFile) {
            }
        }

+        if (sitesOnlyMode) {
+            options.add(Options.DO_NOT_WRITE_GENOTYPES);


Can you check to see whether there are any other GATKTools that bypass GATKTool.createVCFWriter() when writing a VCF and call GATKVariantContextUtils.createVCFWriter() directly instead? (besides the ones you dealt with below)

Don't worry about non-GATKTools.

I have already checked. It is bypassed in a number of tests, the HaplotypeCaller, and CreateSomaticPanelOfNormals which doesn't extend GATKTool so I elected not to include it here.

droazen · 2018-05-11T20:51:16Z

.../java/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/HaplotypeCallerEngine.java

        Utils.nonNull(outputVCF);
        Utils.nonNull(readsDictionary);

+        List<Options> options = new ArrayList<>(2);


droazen · 2018-05-11T20:54:34Z

src/test/java/org/broadinstitute/hellbender/engine/FeatureSupportIntegrationTest.java

@@ -83,6 +92,25 @@ public void testFeaturesAsIntervalsUnrecognizedFormatFile() throws IOException {
        testSpec.executeTest("testFeaturesAsIntervals", this);
    }

+    @Test
+    public void testSitesOnlyMode() {


Can you move this to a new GATKToolIntegrationTest class instead? I regret my original suggestion of FeatureSupportIntegrationTest

droazen · 2018-05-11T20:56:42Z

src/test/java/org/broadinstitute/hellbender/engine/FeatureSupportIntegrationTest.java

+        File out = createTempFile("GTStrippedOutput", "vcf");
+        String[] args = new String[] {
+                "-V",  FEATURE_INTEGRATION_TEST_DIRECTORY + "vcf_with_genotypes.vcf",
+                "--" + StandardArgumentDefinitions.SITES_ONLY_LONG_NAME,


Can you confirm that this test fails if you remove the --sites-only arg temporarily?

just checked, it fails without --sites-only

droazen · 2018-05-11T20:57:16Z

src/test/java/org/broadinstitute/hellbender/engine/FeatureSupportIntegrationTest.java

+            Assert.assertFalse(v.hasGenotypes());
+        }
+    }
+


Could you also add a similar test to HaplotypeCallerIntegrationTest to prove that it honors --sites-only, since that is a special case?

codecov-io · 2018-05-11T21:21:33Z

Codecov Report

Merging #4764 into master will increase coverage by 0.01%.
The diff coverage is 88.889%.

@@             Coverage Diff              @@
##             master    #4764      +/-   ##
============================================
+ Coverage     80.07%   80.08%   +0.01%     
- Complexity    17420    17571     +151     
============================================
  Files          1080     1081       +1     
  Lines         63131    63815     +684     
  Branches      10200    10400     +200     
============================================
+ Hits          50549    51103     +554     
- Misses         8590     8702     +112     
- Partials       3992     4010      +18

Impacted Files	Coverage Δ	Complexity Δ
...ellbender/cmdline/StandardArgumentDefinitions.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...ellbender/utils/test/CommandLineProgramTester.java	`90.909% <0%> (-4.329%)`	`9 <0> (ø)`
...walkers/haplotypecaller/HaplotypeCallerEngine.java	`78.967% <100%> (+0.315%)`	`67 <0> (+1)`	⬆️
...org/broadinstitute/hellbender/engine/GATKTool.java	`91.429% <100%> (+0.124%)`	`93 <0> (ø)`	⬇️
...tools/walkers/haplotypecaller/HaplotypeCaller.java	`93.548% <100%> (ø)`	`16 <0> (ø)`	⬇️
...r/utils/read/markduplicates/sparkrecords/Pair.java	`91.603% <0%> (-0.532%)`	`38% <0%> (+16%)`
...number/gcnv/GermlineCNVSegmentVariantComposer.java	`100% <0%> (ø)`	`10% <0%> (+4%)`	⬆️
...ments/GermlineCNVHybridADVIArgumentCollection.java	`100% <0%> (ø)`	`4% <0%> (+1%)`	⬆️
.../markduplicates/EstimateLibraryComplexityGATK.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...mlineContigPloidyHybridADVIArgumentCollection.java	`100% <0%> (ø)`	`4% <0%> (+1%)`	⬆️
... and 14 more

jamesemery · 2018-05-11T21:21:57Z

@droazen Responded to your comments

droazen

Second-pass review complete, back to @jamesemery

droazen · 2018-05-14T19:24:46Z

src/test/java/org/broadinstitute/hellbender/GatkToolIntegrationTest.java

@@ -0,0 +1,37 @@
+package org.broadinstitute.hellbender;


This class should live in the same package as GATKTool (org.broadinstitute.hellbender.engine)

droazen · 2018-05-14T19:25:25Z

.../java/org/broadinstitute/hellbender/tools/walkers/haplotypecaller/HaplotypeCallerEngine.java

@@ -322,17 +322,20 @@ private void initializeActiveRegionEvaluationGenotyperEngine() {
     * @return a VCF or GVCF writer as appropriate, ready to use
     */
    public VariantContextWriter makeVCFWriter( final String outputVCF, final SAMSequenceDictionary readsDictionary,
-                                               final boolean createOutputVariantIndex, final boolean  createOutputVariantMD5 ) {
+                                               final boolean createOutputVariantIndex, final boolean sitesOnlyMode,
+                                               final boolean  createOutputVariantMD5 ) {


It would be slightly more logical if the ordering of the boolean args were: createOutputVariantIndex, createOutputVariantMD5, sitesOnlyMode instead

droazen · 2018-05-14T19:31:37Z

src/test/java/org/broadinstitute/hellbender/GatkToolIntegrationTest.java

+                "--" + StandardArgumentDefinitions.SITES_ONLY_LONG_NAME,
+                "-O",
+                out.getAbsolutePath()};
+        new Main().instanceMain(makeCommandLineArgs(Arrays.asList(args), SelectVariants.class.getSimpleName()));


Can you add a new overload of runCommandLine() to CommandLineProgramTest that takes 2 arguments: a List of args, and an explicit tool name, and call that new overload here? Should also add it to the CommandLineProgramTester interface.

droazen · 2018-05-14T19:32:25Z

src/test/java/org/broadinstitute/hellbender/engine/FeatureSupportIntegrationTest.java

 import org.broadinstitute.hellbender.CommandLineProgramTest;
+import org.broadinstitute.hellbender.Main;


Remove stray imports from this class

droazen · 2018-05-14T19:33:49Z

.../broadinstitute/hellbender/tools/walkers/haplotypecaller/HaplotypeCallerIntegrationTest.java

@@ -71,6 +73,30 @@ public void testVCFModeIsConsistentWithPastResults(final String inputFileName, f
        IntegrationTestSpec.assertEqualTextFiles(output, expected);
    }

+    @Test
+    public void testSitesOnlyMode() {


Move this test after the standard testVCFMode* and testGVCFMode* tests, rather than sticking it in the middle of them.

Also call Utils.resetRandomGenerator() at the beginning of the test.

droazen · 2018-05-14T19:34:20Z

.../broadinstitute/hellbender/tools/walkers/haplotypecaller/HaplotypeCallerIntegrationTest.java

+        final String[] args = {
+                "-I", NA12878_20_21_WGS_bam,
+                "-R", b37_reference_20_21,
+                "-L", "20:10000000-10100000",


This seems like an unnecessarily large interval for this test. How about 10k bases instead of 100k?

droazen · 2018-05-14T19:36:21Z

src/main/java/org/broadinstitute/hellbender/cmdline/StandardArgumentDefinitions.java

@@ -37,6 +37,7 @@ private StandardArgumentDefinitions(){}
    public static final String ANNOTATIONS_TO_EXCLUDE_LONG_NAME = "annotations-to-exclude";
    public static final String SAMPLE_NAME_LONG_NAME = "sample-name";
    public static final String PEDIGREE_FILE_LONG_NAME = "pedigree";
+    public static final String SITES_ONLY_LONG_NAME = "sites-only";


Can you rename to the more explicit sites-only-vcf-output?

jamesemery · 2018-05-15T15:53:27Z

@droazen Responded to comments

droazen

👍 Will merge once tests pass

added sites-only argument to vcf output files

8f9a0ad

jamesemery assigned droazen May 11, 2018

jamesemery requested review from droazen and ldgauthier May 11, 2018 20:18

ldgauthier approved these changes May 11, 2018

View reviewed changes

droazen requested changes May 11, 2018

View reviewed changes

droazen assigned jamesemery and unassigned droazen May 11, 2018

responded to a round of comments

5157e25

jamesemery assigned droazen and unassigned jamesemery May 14, 2018

droazen requested changes May 14, 2018

View reviewed changes

responding to yet more comments

8f33635

Last few fix-ups

860b6c1

droazen approved these changes May 15, 2018

View reviewed changes

droazen merged commit 4e3affb into master May 17, 2018

droazen deleted the je_sitesOnlyOption branch May 17, 2018 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added --sites-only argument to GATK engine #4764

Added --sites-only argument to GATK engine #4764

jamesemery commented May 11, 2018

jamesemery commented May 11, 2018

ldgauthier commented May 11, 2018

droazen left a comment

droazen May 11, 2018

droazen May 11, 2018

jamesemery May 11, 2018

droazen May 11, 2018

droazen May 11, 2018

droazen May 11, 2018

jamesemery May 11, 2018

droazen May 11, 2018

codecov-io commented May 11, 2018 •

edited

Loading

jamesemery commented May 11, 2018

droazen left a comment

droazen May 14, 2018

jamesemery May 14, 2018

droazen May 14, 2018

jamesemery May 14, 2018

droazen May 14, 2018

jamesemery May 14, 2018

droazen May 14, 2018

jamesemery May 14, 2018

droazen May 14, 2018

droazen May 14, 2018

jamesemery May 14, 2018

droazen May 14, 2018

jamesemery May 14, 2018

droazen May 14, 2018

jamesemery commented May 15, 2018

droazen left a comment

		import org.broadinstitute.hellbender.CommandLineProgramTest;
		import org.broadinstitute.hellbender.Main;

Added --sites-only argument to GATK engine #4764

Added --sites-only argument to GATK engine #4764

Conversation

jamesemery commented May 11, 2018

jamesemery commented May 11, 2018

ldgauthier commented May 11, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 11, 2018 • edited Loading

Codecov Report

jamesemery commented May 11, 2018

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

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

jamesemery commented May 15, 2018

droazen left a comment

Choose a reason for hiding this comment

codecov-io commented May 11, 2018 •

edited

Loading