Simplify HaplotypeBAMWriter code. #944 #5122

kvinter1 · 2018-08-17T19:47:59Z

This is in response to ticket #944

Merged subclass, SAMFileDestination, and superclass, HaplotypeBAMDestination.
Merged both subclasses, AllHaplotypeBAMWriter and CalledHaplotypeBAMWriter, with their superclass, HaplotypeBAMWriter.

codecov-io · 2018-08-17T21:42:39Z

Codecov Report

Merging #5122 into master will increase coverage by 0.029%.
The diff coverage is 100%.

@@              Coverage Diff               @@
##             master     #5122       +/-   ##
==============================================
+ Coverage     86.66%   86.689%   +0.029%     
- Complexity    29043     31076     +2033     
==============================================
  Files          1808      1835       +27     
  Lines        134662    142271     +7609     
  Branches      14935     16228     +1293     
==============================================
+ Hits         116698    123333     +6635     
- Misses        12552     13373      +821     
- Partials       5412      5565      +153

Impacted Files	Coverage Δ	Complexity Δ
...ender/utils/haplotype/HaplotypeBAMDestination.java	`100% <ø> (ø)`	`4 <0> (ø)`	⬇️
...er/utils/haplotype/HaplotypeBAMWriterUnitTest.java	`89.076% <100%> (-0.355%)`	`23 <0> (-1)`
...hellbender/utils/haplotype/HaplotypeBAMWriter.java	`98.305% <100%> (+0.228%)`	`16 <8> (+6)`	⬆️
...kers/haplotypecaller/AssemblyBasedCallerUtils.java	`70.297% <100%> (ø)`	`25 <0> (ø)`	⬇️
...der/tools/spark/sv/discovery/SvDiscoveryUtils.java	`36.082% <0%> (-8.654%)`	`14% <0%> (+1%)`
...rg/broadinstitute/hellbender/utils/Nucleotide.java	`95.266% <0%> (-4.734%)`	`59% <0%> (+52%)`
...ats/collections/SimpleCountCollectionUnitTest.java	`83.784% <0%> (-2.883%)`	`4% <0%> (ø)`
...ngine/spark/AddContextDataToReadSparkUnitTest.java	`92.453% <0%> (-2.142%)`	`12% <0%> (+6%)`
...ils/read/markduplicates/sparkrecords/Fragment.java	`90.909% <0%> (-1.948%)`	`12% <0%> (+6%)`
...r/utils/read/markduplicates/sparkrecords/Pair.java	`98.276% <0%> (-1.724%)`	`51% <0%> (+25%)`
... and 136 more

meganshand · 2018-08-21T19:17:13Z

src/main/java/org/broadinstitute/hellbender/utils/haplotype/HaplotypeBAMDestination.java

+            final String haplotypeReadGroupID)
+    {
+        Utils.nonNull(outPath, "outputPath cannot be null");
+        Utils.nonNull(sourceHeader, "sourceHeader cannot be null");


I think you're checking sourceHeader twice.

Yes, you're right. I will remove the second instance of this.

meganshand · 2018-08-21T19:36:50Z

src/test/java/org/broadinstitute/hellbender/utils/haplotype/HaplotypeBAMWriterUnitTest.java

-        Assert.assertTrue(HaplotypeBAMWriter.create(HaplotypeBAMWriter.WriterType.CALLED_HAPLOTYPES, writer) instanceof CalledHaplotypeBAMWriter);
-        Assert.assertTrue(HaplotypeBAMWriter.create(HaplotypeBAMWriter.WriterType.ALL_POSSIBLE_HAPLOTYPES, writer) instanceof AllHaplotypeBAMWriter);
-    }
-


Did the content of this test get moved to another test?

Nope. This test made sure the two subclasses of HaplotypeBAMWriter (AllHaplotypeBAMWriter and CalledHaplotypeBAMWriter) were indeed subclasses of their superclass. Since these were deleted, there is no more need for the test.

Deletion of redundant check of sourceReader being nonNull. (same as Line 45)

meganshand

Gotcha, looks good to me, but I'd appreciate another set of eyes. Could you take a look at this when you get a chance @cmnbroad?

droazen · 2018-08-22T18:53:35Z

src/main/java/org/broadinstitute/hellbender/utils/haplotype/HaplotypeBAMDestination.java

+    public void add(final GATKRead read){
+        Utils.nonNull(read, "read cannot be null");
+        samWriter.addRead(read);
+    };


There are some extraneous semicolons at the ends of method bodies (here and below). These are not needed now that the methods are no longer abstract.

cmnbroad · 2018-08-27T20:53:20Z

@kvinter1 Back when #944 was originally made, the class hierarchy was overkill since there was only one implementation, but now we have HaplotypeCallerSpark which will need distributed (RDD-based) implementation. So I think we should retain the HaplotypeBAMDestination hierarchy since we'll need it then. Can you revert that part of this (but keep the HaplotypeBAMWriter consolidation part ?). Thanks.

…istence of SAMFileDestination as its subclass. Kept the consolidation of HaplotypeBAMWriter and its subclasses (AllHaplotypeBAMWriter and CalledHaplotypeBAMWriter).

kvinter1 · 2018-08-28T13:46:07Z

@cmnbroad Thanks for the comments. I reverted back to the original, abstract HaplotypeBAMDestination and included its subclass SAMFileDestination. All other changes were kept. Let me know if you find any other issues.

cmnbroad

@kvinter1 Thanks for reverting those changes. I reviewed the remaining code and just have a couple of cleanup comments. thx.

cmnbroad · 2018-08-29T20:52:16Z

src/main/java/org/broadinstitute/hellbender/utils/haplotype/HaplotypeBAMWriter.java

@@ -33,6 +33,7 @@
    private static final int otherMQ = 0;

    protected final HaplotypeBAMDestination output;


output can be made private now

cmnbroad · 2018-08-29T20:58:22Z

src/main/java/org/broadinstitute/hellbender/utils/haplotype/HaplotypeBAMWriter.java

@@ -125,25 +103,48 @@ public void close() {

    /**
     * Write out a BAM representing for the haplotype caller at this site
+     * writerType (ALL_POSSIBLE_HAPLOTYPES or CALLED_HAPLOTYPES) determines inputs to writeHaplotypesAsReads


Lets fix up the javadoc here (the first sentence was incomplete before):

Write out a BAM representing the haplotypes at this site, based on the value for writerType used when the writer was constructed (ALL_POSSIBLE_HAPLOTYPES or CALLED_HAPLOTYPES).

cmnbroad · 2018-08-29T21:01:59Z

src/main/java/org/broadinstitute/hellbender/utils/haplotype/HaplotypeBAMWriter.java

+        Collection<Haplotype> haplotypesToWrite = writerType.equals(WriterType.CALLED_HAPLOTYPES) ? calledHaplotypes : haplotypes;
+        Set<Haplotype> bestHaplotypesToWrite = writerType.equals(WriterType.CALLED_HAPLOTYPES) ? calledHaplotypes : new LinkedHashSet<>(bestHaplotypes);
+
+        writeHaplotypesAsReads(haplotypesToWrite, bestHaplotypesToWrite, paddedReferenceLoc);


I think it would be much clearer to just have an if test after the Utils checks, with one branch for ALL and one for called CALLED.

cmnbroad · 2018-08-29T21:12:20Z

src/main/java/org/broadinstitute/hellbender/utils/haplotype/HaplotypeBAMWriter.java

+        Utils.nonNull(haplotypes, "haplotypes cannot be null");
+        Utils.nonNull(paddedReferenceLoc, "paddedReferenceLoc cannot be null");
+        Utils.nonNull(calledHaplotypes, "calledHaplotypes cannot be null");
+        Utils.nonNull(readLikelihoods, "readLikelihoods cannot be null");


We should add a null check for bestHaplotypes as well. Also, since we're now requiring calledHaplotypes to always be non-null (I think it used to depend on the writer type), lets update the javadoc for all of these args to say they must be non-null.

cmnbroad

Thanks @kvinter1!

kvinter1 added 3 commits August 15, 2018 21:59

Compressed subclass (SAMFileDestination) into HaplotypeBAMDestination.

464a704

Work in progress

51315b7

Attempt to fix #944.

5cbbce3

kvinter1 requested a review from cmnbroad August 17, 2018 19:47

kvinter1 requested a review from meganshand August 20, 2018 13:43

meganshand reviewed Aug 21, 2018

View reviewed changes

Update HaplotypeBAMDestination.java

3348855

Deletion of redundant check of sourceReader being nonNull. (same as Line 45)

meganshand approved these changes Aug 22, 2018

View reviewed changes

droazen reviewed Aug 22, 2018

View reviewed changes

droazen assigned cmnbroad Aug 22, 2018

Fixed some formatting issues

3bc40a3

Reverted back to HaplotypeBAMDestination as abstract super and the ex…

1ba7de7

…istence of SAMFileDestination as its subclass. Kept the consolidation of HaplotypeBAMWriter and its subclasses (AllHaplotypeBAMWriter and CalledHaplotypeBAMWriter).

cmnbroad requested changes Aug 29, 2018

View reviewed changes

kvinter1 added 3 commits August 30, 2018 09:07

Attempt to fix @cmnbroad 's corrections'

b5fa08f

New changes to comments

222ecc0

Changes to if statement in HaplotypeBAMWriter

893d557

cmnbroad approved these changes Aug 30, 2018

View reviewed changes

cmnbroad merged commit da950e2 into master Aug 30, 2018

cmnbroad deleted the kpv_#944 branch August 30, 2018 17:43

kvinter1 mentioned this pull request Aug 30, 2018

Simplify HaplotypeBAMWriter code. #944

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify HaplotypeBAMWriter code. #944 #5122

Simplify HaplotypeBAMWriter code. #944 #5122

kvinter1 commented Aug 17, 2018

codecov-io commented Aug 17, 2018 •

edited

Loading

meganshand Aug 21, 2018

kvinter1 Aug 22, 2018

meganshand Aug 21, 2018

kvinter1 Aug 22, 2018

meganshand left a comment

droazen Aug 22, 2018

cmnbroad commented Aug 27, 2018 •

edited

Loading

kvinter1 commented Aug 28, 2018

cmnbroad left a comment

cmnbroad Aug 29, 2018

cmnbroad Aug 29, 2018

cmnbroad Aug 29, 2018

cmnbroad Aug 29, 2018

cmnbroad left a comment

		@@ -33,6 +33,7 @@
		private static final int otherMQ = 0;

		protected final HaplotypeBAMDestination output;

Simplify HaplotypeBAMWriter code. #944 #5122

Simplify HaplotypeBAMWriter code. #944 #5122

Conversation

kvinter1 commented Aug 17, 2018

codecov-io commented Aug 17, 2018 • edited Loading

Codecov Report

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

meganshand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad commented Aug 27, 2018 • edited Loading

kvinter1 commented Aug 28, 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

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 17, 2018 •

edited

Loading

cmnbroad commented Aug 27, 2018 •

edited

Loading