Updated MarkDuplicates to use Picard metrics code #4779

jamesemery · 2018-05-16T20:33:43Z

lbergelson · 2018-05-17T15:31:11Z

...ain/java/org/broadinstitute/hellbender/utils/read/markduplicates/GATKDuplicationMetrics.java

+ * Metrics that are calculated during the process of marking duplicates
+ * within a stream of SAMRecords.
+ */
+@SuppressWarnings("serial")


Instead of suppressing the serial warning we should add the serialVersionUID. We've had problems in the past due to not including it.

lbergelson · 2018-05-17T15:31:25Z

...ain/java/org/broadinstitute/hellbender/utils/read/markduplicates/GATKDuplicationMetrics.java

+    public void updateMetrics(SAMRecord rec) {
+        update(rec.getReadUnmappedFlag(), rec.isSecondaryOrSupplementary(), ReadUtils.readHasMappedMate(rec), rec.getDuplicateReadFlag() );
+    }
+    public void updateMetrics(GATKRead read) {


needs a linebreak

lbergelson · 2018-05-17T15:37:39Z

.../org/broadinstitute/hellbender/utils/read/markduplicates/ReadEndsForMarkDuplicatesCodec.java

@@ -6,7 +6,7 @@

 import java.io.*;

-/** Coded for ReadEnds that just outputs the primitive fields and reads them back. */
+/** Codec for ReadEnds that just outputs the primitive fields and reads them back. */


lbergelson · 2018-05-17T17:06:05Z

...te/hellbender/tools/walkers/markduplicates/AbstractMarkDuplicatesCommandLineProgramTest.java

@@ -235,28 +237,40 @@ public void testTwoMappedPairsAndMappedSecondaryFragment() {
        tester.addMappedPair(1, 1, 100, false, false, ELIGIBLE_BASE_QUALITY);
        tester.addMappedPair(1, 1, 100, true, true, DEFAULT_BASE_QUALITY); // duplicate!!!
        tester.addMappedFragment(1, 200, false, DEFAULT_BASE_QUALITY, true); // mapped non-primary fragment
+        if (this instanceof MarkDuplicatesSparkIntegrationTest) {


I really don't like these instance of checks . Maybe instead of the instanceof, we can add a method to AbstractMarkDuplicatesTester.doNotMarkUnmappedMates() that will be overriden by the the spark tester to provide the commandline argument.

added an overridden method on AbstractMarkDuplicatesTester to handle it

lbergelson · 2018-05-17T18:15:35Z

...te/hellbender/tools/walkers/markduplicates/AbstractMarkDuplicatesCommandLineProgramTest.java

        tester.addMappedFragment(1, 1, false, ELIGIBLE_BASE_QUALITY);
        tester.addMatePair(1, 200, 0, false, true, false, false, "54M22S", null, false, false, true, true, false, DEFAULT_BASE_QUALITY);
+        if (this instanceof MarkDuplicatesSparkIntegrationTest) {


lbergelson

@jamesemery A few nitpicks with my own code...

jamesemery · 2018-05-18T20:25:04Z

@lbergelson Responded to your comments

lbergelson · 2018-05-18T21:10:02Z

.../org/broadinstitute/hellbender/tools/spark/pipelines/MarkDuplicatesSparkIntegrationTest.java

@@ -42,6 +43,11 @@ protected CommandLineProgram getCommandLineProgramInstance() {
        return new MarkDuplicatesSpark();
    }

+    @Override
+    protected String[] getExtraArguments() {


instead of this, lets just set this always in the tester

codecov-io · 2018-05-18T22:39:46Z

Codecov Report

Merging #4779 into master will increase coverage by 0.095%.
The diff coverage is 87.023%.

@@              Coverage Diff               @@
##             master     #4779       +/-   ##
==============================================
+ Coverage     80.13%   80.225%   +0.095%     
- Complexity    17438     17742      +304     
==============================================
  Files          1082      1085        +3     
  Lines         63150     63834      +684     
  Branches      10187     10353      +166     
==============================================
+ Hits          50602     51211      +609     
- Misses         8564      8606       +42     
- Partials       3984      4017       +33

Impacted Files	Coverage Δ	Complexity Δ
...markduplicates/ReadEndsForMarkDuplicatesCodec.java	`72.34% <ø> (ø)`	`7 <0> (ø)`	⬇️
.../markduplicates/EstimateLibraryComplexityGATK.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...transforms/markduplicates/MarkDuplicatesSpark.java	`98.077% <100%> (+0.16%)`	`19 <11> (+2)`	⬆️
...ates/AbstractMarkDuplicatesCommandLineProgram.java	`93.333% <100%> (ø)`	`24 <5> (ø)`	⬇️
...ls/read/markduplicates/GATKDuplicationMetrics.java	`100% <100%> (ø)`	`13 <13> (?)`
...forms/markduplicates/MarkDuplicatesSparkUtils.java	`89.498% <100%> (-0.29%)`	`62 <4> (-8)`
...roadinstitute/hellbender/utils/read/ReadUtils.java	`81.87% <100%> (+2.652%)`	`341 <9> (+152)`	⬆️
.../utils/read/markduplicates/LibraryIdGenerator.java	`89.474% <100%> (-4.971%)`	`14 <3> (ø)`
...ols/walkers/markduplicates/MarkDuplicatesGATK.java	`87.917% <69.231%> (-2.003%)`	`69 <0> (-7)`
...e/hellbender/utils/test/testers/SamFileTester.java	`85.271% <79.167%> (-5.45%)`	`35 <8> (+3)`
... and 21 more

moving AbstractMarkDuplicateTester to test sources resolves #3762 updating tests to include new tests and changes tests not passing, need to update old gatk test files changed from hellbender...DuplicateMetrics -> hellbender...GATKDuplicateMetrics which extends picard...DuplicateMetrics porting bug fixes to LibraryIdGenerator from picard updating several metrics files with the newest version of picard the lbiraries in the metrics files are then sorted in alphabetical order changes, saving state add addtional test fixes

jamesemery · 2018-05-21T18:05:16Z

@lbergelson Are these changes good for you? I would like to get these changes in soonish.

lbergelson

👍

lbergelson · 2018-05-21T20:51:14Z

@fleharty Did you have any review comments?

fleharty · 2018-05-22T13:12:46Z

I do, haven't finished yet.

…

On Mon, May 21, 2018, 4:51 PM Louis Bergelson ***@***.***> wrote: @fleharty <https://github.com/fleharty> Did you have any review comments? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4779 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKN9DQy1ANY3yfmcfR5Ydvkgj3-SrQ60ks5t0yjLgaJpZM4UB_c2> .

fleharty

@jamesemery Mostly nitpicks here. I approved since this is just really putting in finals and modifying whitespace.

fleharty · 2018-05-21T19:25:17Z

...org/broadinstitute/hellbender/tools/spark/transforms/markduplicates/MarkDuplicatesSpark.java

@@ -1,5 +1,6 @@
 package org.broadinstitute.hellbender.tools.spark.transforms.markduplicates;

+import com.google.common.collect.Iterators;


This import appears to not be used.

I realize it isn't your code, but can you remove the other two rouge imports....
line 21
import org.broadinstitute.hellbender.engine.spark.datasources.ReadsSparkSource;

and
import picard.sam.markduplicates.util.OpticalDuplicateFinder;

fleharty · 2018-05-21T19:30:37Z

...org/broadinstitute/hellbender/tools/spark/transforms/markduplicates/MarkDuplicatesSpark.java

-            return Utils.stream(readsIter).peek(read -> {
+            return Utils.stream(readsIter)
+                    .peek(read -> read.setIsDuplicate(false))
+                    .peek(read -> {
                // Handle reads that have been marked as non-duplicates (which also get tagged with optical duplicate summary statistics)
                if( namesOfNonDuplicateReadsAndOpticalCounts.containsKey(read.getName())) {


spacing around if appears incorrect

fleharty · 2018-05-21T19:34:31Z

...org/broadinstitute/hellbender/tools/spark/transforms/markduplicates/MarkDuplicatesSpark.java

@@ -122,8 +126,10 @@
                    read.setIsDuplicate(false);
                    // Everything else is a duplicate
                } else{


space after else

fleharty · 2018-05-21T19:46:46Z

src/main/java/org/broadinstitute/hellbender/utils/read/ReadUtils.java

@@ -323,6 +323,14 @@ public static boolean readHasMappedMate( final GATKRead read ) {
        return read.isPaired() && ! read.mateIsUnmapped();
    }

+    /**
+     * @param read read to check


This sounds confusing, could you rephrase?

fleharty · 2018-05-21T19:47:53Z

src/main/java/org/broadinstitute/hellbender/utils/read/ReadUtils.java

+     * @param read read to check
+     * @return true if the read is paired and has a mapped mate, otherwise false
+     */
+    public static boolean readHasMappedMate( final SAMRecord read ) {


do spaces go around the argument?

thats a good question, just looking in this file alone it seems to be wholly inconsistent one way or another. I'm not going to bother changing it because it would imply that there was some principled reason for me to unify ALL the formatting in this file at least which I don't necessarily want to do in this PR.

fleharty · 2018-05-21T19:51:12Z

...ain/java/org/broadinstitute/hellbender/utils/read/markduplicates/GATKDuplicationMetrics.java

+    private void update(boolean readUnmappedFlag, boolean secondaryOrSupplementary, boolean mappedMate, boolean isDuplicate) {
+        if (readUnmappedFlag) {
+            ++this.UNMAPPED_READS;
+        } else if(secondaryOrSupplementary) {


space after if

fleharty · 2018-05-21T19:52:47Z

src/main/java/org/broadinstitute/hellbender/utils/read/markduplicates/LibraryIdGenerator.java


    public Histogram<Short> getOpticalDuplicatesByLibraryIdMap() { return this.opticalDuplicatesByLibraryId; }

+	public static String getReadGroupLibraryName(SAMReadGroupRecord readGroup) {


final SAMReadGroupRecord?

fleharty · 2018-05-21T19:54:56Z

src/main/java/org/broadinstitute/hellbender/utils/test/testers/SamFileTester.java

@@ -23,17 +24,29 @@
    private boolean deleteOnExit = true;
    protected final List<String> args = new ArrayList<>();

-    public SamFileTester(final int readLength, final boolean deleteOnExit, final int defaultChromosomeLength, final DuplicateScoringStrategy.ScoringStrategy duplicateScoringStrategy) {
+    public SamFileTester(final int readLength, final boolean deleteOnExit, final int defaultChromosomeLength, final DuplicateScoringStrategy.ScoringStrategy duplicateScoringStrategy, final SAMFileHeader.SortOrder sortOrder, boolean recordsNeedSorting) {


final boolean?

fleharty · 2018-05-22T15:15:03Z

...test/java/org/broadinstitute/hellbender/utils/test/testers/AbstractMarkDuplicatesTester.java

 public abstract class AbstractMarkDuplicatesTester extends SamFileTester {

    final private File metricsFile;
    final DuplicationMetrics expectedMetrics;

-    public AbstractMarkDuplicatesTester(final ScoringStrategy duplicateScoringStrategy) {
-        super(50, true, SAMRecordSetBuilder.DEFAULT_CHROMOSOME_LENGTH, duplicateScoringStrategy);
+    public AbstractMarkDuplicatesTester(final ScoringStrategy duplicateScoringStrategy, SAMFileHeader.SortOrder sortOrder) {


missing finals?

fleharty · 2018-05-22T15:15:14Z

...test/java/org/broadinstitute/hellbender/utils/test/testers/AbstractMarkDuplicatesTester.java

+        this(duplicateScoringStrategy, sortOrder, true);
+    }
+
+    public AbstractMarkDuplicatesTester(final ScoringStrategy duplicateScoringStrategy, SAMFileHeader.SortOrder sortOrder, boolean recordNeedSorting) {


missing finals?

jamesemery · 2018-05-22T16:32:35Z

@fleharty Thank you for the review.

jamesemery · 2018-05-22T17:39:17Z

@fleharty The reason for the "read read" has to do with the javadocs. The first one is a flag indicating the argument name for documentation purposes. The second one is just the description, which should be reasonably formatted in the docs compared to how it looks here on github.

fleharty · 2018-05-22T17:48:35Z

@jamesemery I see, of course... for some reason my eye didn't catch that the first "read" was the parameter, and the second "read" was part of the description. Thanks for the clarification.

jamesemery assigned lbergelson May 16, 2018

jamesemery requested a review from lbergelson May 16, 2018 20:33

lbergelson reviewed May 17, 2018

View reviewed changes

lbergelson requested changes May 17, 2018

View reviewed changes

lbergelson assigned jamesemery and unassigned lbergelson May 17, 2018

jamesemery assigned lbergelson and unassigned jamesemery May 18, 2018

lbergelson reviewed May 18, 2018

View reviewed changes

jamesemery force-pushed the lb_update_md branch from 67bb3b0 to 50db8bb Compare May 18, 2018 21:39

lbergelson and others added 7 commits May 21, 2018 10:05

Updating to work properly with the newer MarkDuplicatesSpark code

83aae26

did something?

b9eaade

responded to comments

0be29f0

Cleaned up the tests

3514193

using a slightly better annotation

e21f9bd

Resolving merge conflicts

66e477e

jamesemery force-pushed the lb_update_md branch from 50db8bb to 66e477e Compare May 21, 2018 14:05

lbergelson approved these changes May 21, 2018

View reviewed changes

jamesemery assigned mwalker174 and fleharty and unassigned mwalker174 May 21, 2018

fleharty approved these changes May 22, 2018

View reviewed changes

responding to another round of comments

6c23b51

jamesemery merged commit 2cc7abd into master May 22, 2018

lbergelson deleted the lb_update_md branch May 25, 2018 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated MarkDuplicates to use Picard metrics code #4779

Updated MarkDuplicates to use Picard metrics code #4779

jamesemery commented May 16, 2018

lbergelson May 17, 2018

jamesemery May 18, 2018

lbergelson May 17, 2018

lbergelson May 17, 2018

lbergelson May 17, 2018

jamesemery May 18, 2018

lbergelson May 17, 2018

lbergelson left a comment

jamesemery commented May 18, 2018

lbergelson May 18, 2018

codecov-io commented May 18, 2018 •

edited

Loading

jamesemery commented May 21, 2018

lbergelson left a comment

lbergelson commented May 21, 2018

fleharty commented May 22, 2018 via email

fleharty left a comment

fleharty May 21, 2018

fleharty May 21, 2018

jamesemery May 22, 2018

fleharty May 21, 2018

fleharty May 21, 2018

fleharty May 21, 2018

fleharty May 21, 2018

jamesemery May 22, 2018

fleharty May 21, 2018

fleharty May 21, 2018

fleharty May 21, 2018

fleharty May 22, 2018

fleharty May 22, 2018

jamesemery commented May 22, 2018

jamesemery commented May 22, 2018

fleharty commented May 22, 2018

		@@ -1,5 +1,6 @@
		package org.broadinstitute.hellbender.tools.spark.transforms.markduplicates;

		import com.google.common.collect.Iterators;


		public Histogram<Short> getOpticalDuplicatesByLibraryIdMap() { return this.opticalDuplicatesByLibraryId; }

		public static String getReadGroupLibraryName(SAMReadGroupRecord readGroup) {

Updated MarkDuplicates to use Picard metrics code #4779

Updated MarkDuplicates to use Picard metrics code #4779

Conversation

jamesemery commented May 16, 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

lbergelson left a comment

Choose a reason for hiding this comment

jamesemery commented May 18, 2018

Choose a reason for hiding this comment

codecov-io commented May 18, 2018 • edited Loading

Codecov Report

jamesemery commented May 21, 2018

lbergelson left a comment

Choose a reason for hiding this comment

lbergelson commented May 21, 2018

fleharty commented May 22, 2018 via email

fleharty 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

jamesemery commented May 22, 2018

jamesemery commented May 22, 2018

fleharty commented May 22, 2018

codecov-io commented May 18, 2018 •

edited

Loading