2 pass variant walker #4744

takutosato · 2018-05-07T20:31:41Z

No description provided.

takutosato · 2018-05-07T20:32:26Z

Hey @droazen will you review this?

cmnbroad

This will be useful to have. Requested javadoc and unit tests.

cmnbroad · 2018-05-08T12:47:54Z

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

+
+import java.util.stream.StreamSupport;
+
+public abstract class TwoPassVariantWalker extends VariantWalker {


This is great - we can use this in the CNNScoreVariants tool (#4316). It will need class and method javadoc though, and unit tests (see the ones for TwoPassReadWalker).

cmnbroad · 2018-05-08T12:54:43Z

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

+        // Second pass
+        logger.info("Starting second pass through the variants");
+        traverseVariants(variantFilter, readFilter, this::secondPassApply);
+        logger.info(readFilter.getSummaryLine());


This accumulates the filter count across both passes and displays the aggregate total. The TwoPassReadWalker does the same thing, though I wonder if thats a feature, given that the second pass is usually an implementation detail.

I talked to Louis and James a bit about this. I don't think we need two different VariantFilters, so I'll just call makeVariantFilter() twice, in the hope that calling it the second time resets the counter

droazen

Review complete, back to @takutosato. This looks good, just needs some tests and javadoc.

droazen · 2018-05-08T15:04:29Z

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

+
+import java.util.stream.StreamSupport;
+
+public abstract class TwoPassVariantWalker extends VariantWalker {


For tests, I think we need 3 things here:

A TwoPassVariantWalkerUnitTest test class, modeled after TwoPassReadWalkerUnitTest

An ExampleTwoPassVariantWalker tool in tools.examples, modeled after the existing ExampleVariantWalker tool

A simple ExampleTwoPassVariantWalkerIntegrationTest, modeled after ExampleVariantWalkerIntegrationTest

The unit test for TwoPassVariantWalker writes a private example walker class and runs it, which seems to be the same as the integration test. I think doing either is sufficient - what do you think?

It's good to have both the unit test and the integration test. The unit test uses a toy implementation to count calls to first/second pass apply, which is a good correctness test for the traversal. However, we also try to include a runnable example implementation that actually produces meaningful output for every walker type as a template for tool developers, and that's where ExampleTwoPassVariantWalker comes in.

droazen · 2018-05-08T15:27:15Z

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

+
+    @Override
+    public void traverse(){
+        final VariantFilter variantFilter = makeVariantFilter();


Can you file a github ticket saying that VariantWalkerBase (and subclasses) need to migrate to CountingVariantFilter, so that summary statistics for variant filtration can be printed at the end of traversal, as they are for read filters currently?

@cmnbroad Do you know why CountingVariantFilter never got hooked up here?

I'm not sure why we didn't do that.

droazen · 2018-05-08T15:28:38Z

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

+    }
+
+
+    protected abstract void firstPassApply(final VariantContext variant,


Add full javadoc for firstPassApply(), secondPassApply(), and afterFirstPass(), modeled after the javadoc for the analogous methods in TwoPassReadWalker

codecov-io · 2018-05-09T17:03:50Z

Codecov Report

Merging #4744 into master will increase coverage by 0.215%.
The diff coverage is 91.667%.

@@               Coverage Diff               @@
##              master     #4744       +/-   ##
===============================================
+ Coverage     79.985%   80.201%   +0.215%     
- Complexity     17419     18234      +815     
===============================================
  Files           1081      1086        +5     
  Lines          63118     66825     +3707     
  Branches       10195     11063      +868     
===============================================
+ Hits           50485     53594     +3109     
- Misses          8648      9073      +425     
- Partials        3985      4158      +173

Impacted Files	Coverage Δ	Complexity Δ
...er/tools/examples/ExampleTwoPassVariantWalker.java	`89.286% <89.286%> (ø)`	`8 <8> (?)`
...titute/hellbender/engine/TwoPassVariantWalker.java	`95% <95%> (ø)`	`4 <4> (?)`
...ery/inference/SimpleNovelAdjacencyInterpreter.java	`80.435% <0%> (-19.565%)`	`15% <0%> (+8%)`
.../DiscoverVariantsFromContigAlignmentsSAMSpark.java	`88.889% <0%> (-4.659%)`	`49% <0%> (+27%)`
...ellbender/utils/test/CommandLineProgramTester.java	`91.667% <0%> (-3.571%)`	`11% <0%> (+2%)`
...r/utils/read/markduplicates/sparkrecords/Pair.java	`93.893% <0%> (-1.612%)`	`44% <0%> (+22%)`
...iscoverFromLocalAssemblyContigAlignmentsSpark.java	`76.048% <0%> (-0.474%)`	`5% <0%> (ø)`
...lbender/tools/spark/sv/discovery/SimpleSVType.java	`86% <0%> (-0.441%)`	`3% <0%> (+1%)`
.../discovery/inference/ImpreciseVariantDetector.java	`80.952% <0%> (-0.298%)`	`6% <0%> (ø)`
...itute/hellbender/engine/spark/GATKRegistrator.java	`100% <0%> (ø)`	`6% <0%> (+3%)`	⬆️
... and 57 more

takutosato · 2018-05-10T17:59:40Z

@droazen (or @cmnbroad?) back to you

droazen · 2018-05-14T19:54:45Z

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

+     * @param variant A variant record in a vcf
+     * @param readsContext Reads overlapping the current variant. Will be empty if a read source (e.g. bam) isn't provided
+     * @param referenceContext Reference bases spanning the current variant
+     * @param featureContext A vcf record overlapping the current variant from an auxiliary data source (e.g. gnomad)


featureContext supports more than just VCF records as auxiliary inputs -- any tribble-supported format can be accepted. I'd just change vcf record to record

droazen · 2018-05-14T19:55:50Z

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

+
+    /**
+     *
+     * First pass through the variants. The user may store data in instance variables of the walker as you go


as you go -> as they go

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

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

+        final VariantFilter variantContextFilter = makeVariantFilter();
+        final CountingReadFilter readFilter = makeReadFilter();
+
+        // First pass through the variant


variant -> variants

droazen · 2018-05-14T19:58:22Z

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

+                                            final FeatureContext featureContext);
+
+    /**
+     * Process the data collected during the first pass


Add explicit comment that this is called after firstPassApply() and before secondPassApply()

droazen · 2018-05-14T20:00:36Z

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

+        // Second pass
+        logger.info("Starting second pass through the variants");
+        traverseVariants(variantContextFilter, readFilter, this::secondPassApply);
+    }


Add logger.info(readFilter.getSummaryLine()) after the second traversal pass.

droazen · 2018-05-14T20:01:56Z

src/main/java/org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalker.java

+import java.util.ArrayList;
+import java.util.List;
+
+import static org.broadinstitute.hellbender.utils.variant.GATKVCFConstants.QUAL_BY_DEPTH_KEY;


Avoid static imports -- they tend to create confusion when reading the code.

droazen · 2018-05-14T20:02:52Z

src/main/java/org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalker.java

+
+@CommandLineProgramProperties(
+        summary = "Example variant walker that makes two passes through a vcf",
+        oneLineSummary = "Example two variant walker",


"two variant" -> "two-pass variant"

droazen · 2018-05-14T20:04:15Z

src/main/java/org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalker.java

+
+    private double sampleVarianceOfQDs;
+
+    int counter = 0;


droazen · 2018-05-14T20:06:26Z

src/main/java/org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalker.java

+    }
+
+    @Override
+    protected void afterFirstPass() {


Example code would be more logical if you put afterFirstPass() physically between firstPassApply() and secondPassApply()

droazen · 2018-05-14T20:07:13Z

src/main/java/org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalker.java

+import java.util.List;
+
+import static org.broadinstitute.hellbender.utils.variant.GATKVCFConstants.QUAL_BY_DEPTH_KEY;
+


Document in a 1-2 sentence comment what the example tool does.

droazen · 2018-05-14T20:07:37Z

...org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalkerIntegrationTest.java

+
+import static org.broadinstitute.hellbender.tools.examples.ExampleTwoPassVariantWalker.COPY_OF_QD_KEY_NAME;
+import static org.broadinstitute.hellbender.tools.examples.ExampleTwoPassVariantWalker.QD_DISTANCE_FROM_MEAN;
+import static org.broadinstitute.hellbender.utils.variant.GATKVCFConstants.QUAL_BY_DEPTH_KEY;


Avoid static imports

droazen · 2018-05-14T20:08:48Z

...org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalkerIntegrationTest.java

+public class ExampleTwoPassVariantWalkerIntegrationTest extends CommandLineProgramTest {
+    @Test
+    public void test() throws IOException {
+        final File outputVcf = File.createTempFile("output", "vcf");


Use BaseTest.createTempFile() instead, as it schedules the file (+ companion indices) for deletion on JVM exit.

droazen · 2018-05-14T20:10:24Z

...org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalkerIntegrationTest.java

+        runCommandLine(args);
+
+        final FeatureDataSource<VariantContext> variantsBefore = new FeatureDataSource<>(inputVcf);
+        final FeatureDataSource<VariantContext> variantsAfter = new FeatureDataSource<>(outputVcf);


Create these data sources in a try-with-resources block, to ensure that they get closed.

droazen · 2018-05-14T20:11:27Z

...org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalkerIntegrationTest.java

+    @Test
+    public void test() throws IOException {
+        final File outputVcf = File.createTempFile("output", "vcf");
+        final String inputVcf = "src/test/resources/org/broadinstitute/hellbender/tools/walkers/variantutils/SelectVariants/haploid-multisample.vcf";


Is this really a good VCF to use for this test?

That was the first vcf that had QD - most vcfs in the test suite don't

droazen · 2018-05-14T20:13:25Z

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

+    @CommandLineProgramProperties(
+            summary = "An example subclass of TwoPassVariantWalker",
+            oneLineSummary = "An example subclass of TwoPassVariantWalker",
+            programGroup = TestProgramGroup.class


omitFromCommandLine = true

droazen · 2018-05-14T20:13:56Z

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

+    }
+
+    @Test
+    public void testNum() {


testNum -> testTwoPassTraversal()

droazen · 2018-05-14T20:17:45Z

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

+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class TwoPassVariantWalkerUnitTest {


extends GATKBaseTest

droazen

Second-pass review complete, back to @takutosato with more comments.

takutosato · 2018-05-15T13:50:39Z

@droazen thanks for the review. back to you

droazen

Final review complete, back to @takutosato. Two last quick comments to address, then go ahead and hit merge after tests pass.

droazen · 2018-05-15T20:54:16Z

src/main/java/org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalker.java

+
+    private VariantContextWriter vcfWriter;
+
+    static String QD_DISTANCE_FROM_MEAN = "QD_DIST";


As a constant this should be static final, not just static

droazen · 2018-05-15T20:54:26Z

src/main/java/org/broadinstitute/hellbender/tools/examples/ExampleTwoPassVariantWalker.java

+
+    static String QD_DISTANCE_FROM_MEAN = "QD_DIST";
+
+    static String COPY_OF_QD_KEY_NAME = "QD_COPY";


This should also be static final

droazen · 2018-05-17T20:05:10Z

I pushed the final requested changes myself just now -- will merge once tests pass.

All comments addressed

takutosato · 2018-05-17T20:32:31Z

Great, thanks @droazen!

2 pass variant walker class

a7fada1

takutosato requested a review from droazen May 7, 2018 20:31

cmnbroad previously requested changes May 8, 2018

View reviewed changes

droazen self-assigned this May 8, 2018

droazen suggested changes May 8, 2018

View reviewed changes

droazen assigned takutosato and unassigned droazen May 8, 2018

takutosato added 3 commits May 10, 2018 11:44

wrote docs, tests

1910651

edit the docs

e002c1b

update test

a673774

droazen assigned droazen and unassigned takutosato May 14, 2018

droazen reviewed May 14, 2018

View reviewed changes

droazen suggested changes May 14, 2018

View reviewed changes

takutosato added 2 commits May 15, 2018 09:47

review edits

02253f3

reword

067f6fe

droazen approved these changes May 15, 2018

View reviewed changes

droazen assigned takutosato and unassigned droazen May 15, 2018

static -> static final

147f65e

droazen assigned droazen and unassigned takutosato May 17, 2018

droazen merged commit 6e1cc8c into master May 17, 2018

droazen deleted the ts_2pass branch May 17, 2018 21:47


		import java.util.stream.StreamSupport;

		public abstract class TwoPassVariantWalker extends VariantWalker {

		}


		protected abstract void firstPassApply(final VariantContext variant,

		import java.util.List;

		import static org.broadinstitute.hellbender.utils.variant.GATKVCFConstants.QUAL_BY_DEPTH_KEY;


		private VariantContextWriter vcfWriter;

		static String QD_DISTANCE_FROM_MEAN = "QD_DIST";


		static String QD_DISTANCE_FROM_MEAN = "QD_DIST";

		static String COPY_OF_QD_KEY_NAME = "QD_COPY";

2 pass variant walker #4744

2 pass variant walker #4744

Conversation

takutosato commented May 7, 2018

takutosato commented May 7, 2018

cmnbroad 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

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

codecov-io commented May 9, 2018 • edited Loading

Codecov Report

takutosato commented May 10, 2018

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

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 left a comment • edited Loading

Choose a reason for hiding this comment

takutosato commented May 15, 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

droazen commented May 17, 2018

takutosato commented May 17, 2018

codecov-io commented May 9, 2018 •

edited

Loading

droazen left a comment •

edited

Loading