the long awaited FastaAlternateReferenceMaker #5549

lbergelson · 2018-12-21T22:41:16Z

new walker type ReferenceWalker
changes to close behavior in FastaWriter and adding additional buffering
new walkers:
CountBasesInReference
FastaReferenceMaker
FastaAlternateReferenceMaker

codecov-io · 2018-12-21T23:47:33Z

Codecov Report

Merging #5549 into master will decrease coverage by 0.072%.
The diff coverage is 92.824%.

@@              Coverage Diff               @@
##             master     #5549       +/-   ##
==============================================
- Coverage     87.09%   87.018%   -0.072%     
- Complexity    31524     31706      +182     
==============================================
  Files          1930      1943       +13     
  Lines        145231    146064      +833     
  Branches      16095     16137       +42     
==============================================
+ Hits         126482    127102      +620     
- Misses        12900     13077      +177     
- Partials       5849      5885       +36

Impacted Files	Coverage Δ	Complexity Δ
...institute/hellbender/utils/help/HelpConstants.java	`4.167% <ø> (ø)`	`1 <0> (ø)`	⬇️
...roadinstitute/hellbender/utils/SimpleInterval.java	`93.182% <ø> (ø)`	`48 <0> (ø)`	⬇️
...dinstitute/hellbender/engine/ReferenceContext.java	`83.908% <ø> (ø)`	`39 <0> (ø)`	⬇️
...lbender/utils/iterators/IntervalLocusIterator.java	`92.593% <ø> (ø)`	`11 <0> (ø)`	⬇️
...org/broadinstitute/hellbender/utils/BaseUtils.java	`87.903% <100%> (+1.178%)`	`56 <10> (+10)`	⬆️
...lbender/tools/examples/ExampleReferenceWalker.java	`100% <100%> (ø)`	`7 <7> (?)`
...rs/fasta/CountBasesInReferenceIntegrationTest.java	`100% <100%> (ø)`	`3 <3> (?)`
...llbender/utils/reference/FastaReferenceWriter.java	`92.466% <100%> (+0.052%)`	`51 <0> (ø)`	⬇️
...xamples/ExampleReferenceWalkerIntegrationTest.java	`100% <100%> (ø)`	`3 <3> (?)`
...lkers/variantutils/ReblockGVCFIntegrationTest.java	`97.826% <100%> (ø)`	`8 <0> (ø)`	⬇️
... and 147 more

* new walker type ReferenceWalker * changes to close behavior in FastaWriter and adding additional buffering * new walkers: CountBasesInReference FastaReferenceMaker FastaAlternateReferenceMaker

droazen

Minor comments only -- back to @lbergelson. Merge once these are addressed.

droazen · 2019-01-09T19:05:12Z

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

+import org.broadinstitute.hellbender.utils.iterators.IntervalLocusIterator;
+
+/**
+ * A reference walker is a tool which process each position in a given reference.


process -> processes

each position -> each base that overlaps a set of intervals? (ie., describe the traversal more precisely here)

droazen · 2019-01-09T19:11:03Z

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

+ * A reference walker is a tool which process each position in a given reference.
+ *
+ *  ReferenceWalker authors must implement the apply() method to process each position, and may optionally implement
+ *  {@link #onTraversalStart()} and/or {@link #onTraversalSuccess()}. See the {@link ExampleReferenceWalker} walker for an example.


Mention getReferenceWindow() here as an additional method that tool authors might want to override.

droazen · 2019-01-09T19:17:18Z

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

+
+    /**
+     * Determine the window to use when creating the ReferenceContext in apply.  This determines which reference bases are
+     * see at each position, as well as which reads / features are considered to overlap the reference site.


see -> seen

droazen · 2019-01-09T19:36:27Z

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

+        final CountingReadFilter readFilter = makeReadFilter();
+
+        for(SimpleInterval locus : getIntervalIterator()){
+            SimpleInterval referenceWindow = getReferenceWindow(locus);


locus and referenceWindow should both be final

huh, I wonder why intellij didn't remind me of that.

droazen · 2019-01-28T20:04:22Z

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

@@ -0,0 +1,78 @@
+package org.broadinstitute.hellbender.tools.examples;
+
+import com.netflix.servo.util.VisibleForTesting;


Wrong VisibleForTesting annotation...

changed to com.hulu.util.VisibleForTesting

droazen · 2019-01-29T16:48:38Z

.../org/broadinstitute/hellbender/tools/walkers/fasta/CountBasesInReferenceIntegrationTest.java

+public class CountBasesInReferenceIntegrationTest extends CommandLineProgramTest {
+
+    @Test
+    public void testExampleReferenceWalker(){


testExampleReferenceWalker() -> testCountBasesInReferenceWithInterval()

droazen · 2019-01-29T16:49:55Z

.../org/broadinstitute/hellbender/tools/walkers/fasta/CountBasesInReferenceIntegrationTest.java

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


testExampleReferenceWalkerFull() -> testCountBasesInReferenceFull()

droazen · 2019-01-29T16:57:12Z

src/test/java/org/broadinstitute/hellbender/utils/BaseUtilsUnitTest.java

+        Assert.assertEquals(BaseUtils.basesToIUPAC((byte)'A', (byte)'T'), 'W', "testing A/T=W");
+        Assert.assertEquals(BaseUtils.basesToIUPAC((byte)'T', (byte)'A'), 'W', "testing T/A=W");
+        Assert.assertEquals(BaseUtils.basesToIUPAC((byte) 'G', (byte) 'T'), 'K', "testing G/T=K");
+        Assert.assertEquals(BaseUtils.basesToIUPAC((byte) 'T', (byte) 'G'), 'K', "testing T/G=K");


This test doesn't cover all the possibilities -- could you add the missing cases? (should be pretty easy)

droazen · 2019-01-29T17:03:02Z

src/testUtils/java/org/broadinstitute/hellbender/testutils/FastaTestUtils.java

+import java.util.Arrays;
+import java.util.Map;
+
+public class FastaTestUtils {


final (and make the default constructor private)

droazen · 2019-01-29T17:04:42Z

src/testUtils/java/org/broadinstitute/hellbender/testutils/FastaTestUtils.java

+
+public class FastaTestUtils {
+
+    public static void assertOutput(final Path path, final boolean mustHaveIndex, final boolean mustHaveDictionary,


Add docs for these methods (and give assertOutput() a more descriptive name)

I moved most of these back to the unit test they originally came from. I had intended to use them in my tests but they ended up being to brittle to adapt. I documented what was left.

droazen self-requested a review January 3, 2019 19:19

droazen self-assigned this Jan 3, 2019

lbergelson force-pushed the lb_fasta_alternative_reference branch from 4ec7a34 to f98fa90 Compare January 3, 2019 19:38

lbergelson added 2 commits January 9, 2019 13:44

literal copy of gatk3 version

31f7331

Port FastaAlternateReferenceMaker

4e60787

* new walker type ReferenceWalker * changes to close behavior in FastaWriter and adding additional buffering * new walkers: CountBasesInReference FastaReferenceMaker FastaAlternateReferenceMaker

lbergelson force-pushed the lb_fasta_alternative_reference branch from f98fa90 to 4e60787 Compare January 9, 2019 18:46

droazen approved these changes Jan 29, 2019

View reviewed changes

droazen assigned lbergelson and unassigned droazen Jan 29, 2019

responding to comments

b23e257

droazen merged commit 64e2d3a into master Jan 29, 2019

droazen deleted the lb_fasta_alternative_reference branch January 29, 2019 22:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

the long awaited FastaAlternateReferenceMaker #5549

the long awaited FastaAlternateReferenceMaker #5549

lbergelson commented Dec 21, 2018

codecov-io commented Dec 21, 2018 •

edited

Loading

droazen left a comment

droazen Jan 9, 2019

droazen Jan 9, 2019

lbergelson Jan 29, 2019

droazen Jan 9, 2019

lbergelson Jan 29, 2019

droazen Jan 9, 2019

lbergelson Jan 29, 2019

droazen Jan 9, 2019

lbergelson Jan 29, 2019

droazen Jan 28, 2019

lbergelson Jan 29, 2019

droazen Jan 29, 2019

droazen Jan 29, 2019

droazen Jan 29, 2019

lbergelson Jan 29, 2019

droazen Jan 29, 2019

droazen Jan 29, 2019

lbergelson Jan 29, 2019

		@@ -0,0 +1,78 @@
		package org.broadinstitute.hellbender.tools.examples;

		import com.netflix.servo.util.VisibleForTesting;


		public class FastaTestUtils {

		public static void assertOutput(final Path path, final boolean mustHaveIndex, final boolean mustHaveDictionary,

the long awaited FastaAlternateReferenceMaker #5549

the long awaited FastaAlternateReferenceMaker #5549

Conversation

lbergelson commented Dec 21, 2018

codecov-io commented Dec 21, 2018 • edited Loading

Codecov Report

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

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 Dec 21, 2018 •

edited

Loading