Updated MarkDuplicates code to rely on the Picard OpticalDuplicatesFinder #4750

jamesemery · 2018-05-08T19:10:57Z

lbergelson · 2018-05-09T17:58:13Z

...roadinstitute/hellbender/tools/spark/transforms/markduplicates/MarkDuplicatesSparkUtils.java

@@ -86,6 +92,9 @@ public int getIndex() {

        final JavaPairRDD<String, Iterable<IndexPair<GATKRead>>> keyedReads = getReadsGroupedByName(header, mappedReads, numReducers);

+        //TODO why in the world does a JavaRDD return a normal sparkcontext instead of the nice wrapped one...?
+        final Broadcast<Map<String, Short>> headerReadGroupIndexMap = new JavaSparkContext(reads.context()).broadcast( getHeaderReadGroupIndexMap(header));


use JavaSparkContext.fromSparkContext instead I think.

lbergelson · 2018-05-09T19:29:13Z

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

-                prevIdx = i + 1;
-            }
-        }
-        if (prevIdx < readName.length()) {


YAY deleting code!

lbergelson · 2018-05-09T19:29:52Z

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


    @Override
-    public void setY(final short y) { this.y = y; }
+    public void setY(final int y) { this.y = (short)y; }


We should add checks here that it's not > maxshort.

lbergelson · 2018-05-09T19:30:24Z

...broadinstitute/hellbender/utils/read/markduplicates/SerializableOpticalDuplicatesFinder.java

+
+/**
+ * An intermediate class flagged as being serializable so that the Picard OpticalDuplicateFinder can be serialized despite
+ * it not being marked as such. There is not state outside of construction at the time of creating this pr.


the second sentence is more confusing than helpful

lbergelson

@jamesemery A few minor changes and typos. Looks good though. Surprisingly little change.

lbergelson · 2018-05-09T19:31:02Z

...rg/broadinstitute/hellbender/tools/walkers/markduplicates/EstimateLibraryComplexityGATK.java

@@ -142,11 +142,11 @@
    /**
     * Little class to hold the sequence of a pair of reads and tile location information.
     */
-    static class PairedReadSequence implements OpticalDuplicateFinder.PhysicalLocation {
+    static class PairedReadSequence implements picard.sam.util.PhysicalLocation {


do we need the full path here?

lbergelson · 2018-05-09T19:31:13Z

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

@@ -1,7 +1,7 @@
 package org.broadinstitute.hellbender.utils.read.markduplicates;

 /** Little struct-like class to hold read pair (and fragment) end data for duplicate marking. */
-public abstract class ReadEnds implements OpticalDuplicateFinder.PhysicalLocation {
+public abstract class ReadEnds implements picard.sam.util.PhysicalLocation {


shorten the path?

lbergelson · 2018-05-09T19:31:24Z

src/main/java/org/broadinstitute/hellbender/utils/read/markduplicates/sparkrecords/Pair.java

@@ -20,7 +19,7 @@
 * during the processing step of MarkDuplicatesSpark
 */
 @DefaultSerializer(Pair.Serializer.class)
-public final class Pair extends PairedEnds implements OpticalDuplicateFinder.PhysicalLocation {
+public final class Pair extends PairedEnds implements picard.sam.util.PhysicalLocation {


short path?

lbergelson · 2018-05-09T19:32:50Z

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

@@ -225,6 +230,11 @@ public void addMatePair(final String readName,
            record2.setAttribute("MC", null);
        }

+        if (readGorup!=null) {


typo: readGorup

lbergelson · 2018-05-09T19:33:51Z

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

+    public void testOpticalDuplicatesDifferentReadGroups() {
+        final AbstractMarkDuplicatesTester tester = getTester();
+        tester.setExpectedOpticalDuplicate(0);
+        tester.addMatePair("RUNID:7:1203:2886:82292",  19, 19, 485253, 485253, false, false, true, true, "42M59S", "59S42M", true, false, false, false, false, DEFAULT_BASE_QUALITY, "H0164.2");  // duplicate


I hate these 600 parameter methods... but I'm not sure what to do about them

there are a bunch of overloads that help the problem. For the most par every field is necessary to at least one of these tests.

jamesemery · 2018-05-09T20:12:01Z

@lbergelson Responded to your comments.

codecov-io · 2018-05-09T22:52:55Z

Codecov Report

Merging #4750 into master will increase coverage by 0.069%.
The diff coverage is 87.037%.

@@               Coverage Diff               @@
##              master     #4750       +/-   ##
===============================================
+ Coverage     80.052%   80.121%   +0.069%     
- Complexity     17413     17492       +79     
===============================================
  Files           1080      1080               
  Lines          63099     63440      +341     
  Branches       10196     10275       +79     
===============================================
+ Hits           50512     50829      +317     
- Misses          8597      8607       +10     
- Partials        3990      4004       +14

Impacted Files	Coverage Δ	Complexity Δ
...transforms/markduplicates/MarkDuplicatesSpark.java	`95.918% <ø> (ø)`	`16 <0> (ø)`	⬇️
...llections/OpticalDuplicatesArgumentCollection.java	`100% <ø> (ø)`	`1 <0> (ø)`	⬇️
...icates/sparkrecords/MarkDuplicatesSparkRecord.java	`100% <ø> (ø)`	`7 <0> (ø)`	⬇️
...tractOpticalDuplicateFinderCommandLineProgram.java	`100% <ø> (ø)`	`4 <0> (ø)`	⬇️
.../markduplicates/EstimateLibraryComplexityGATK.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...hellbender/utils/read/markduplicates/ReadEnds.java	`92% <100%> (ø)`	`15 <5> (ø)`	⬇️
...r/utils/read/markduplicates/sparkrecords/Pair.java	`93.333% <100%> (+1.199%)`	`23 <7> (+1)`	⬆️
...forms/markduplicates/MarkDuplicatesSparkUtils.java	`90% <100%> (+0.36%)`	`69 <8> (+4)`	⬆️
...ates/AbstractMarkDuplicatesCommandLineProgram.java	`93.333% <100%> (ø)`	`24 <4> (ø)`	⬇️
...k/pipelines/BwaAndMarkDuplicatesPipelineSpark.java	`77.778% <100%> (ø)`	`4 <0> (ø)`	⬇️
... and 13 more

… marking code

jamesemery · 2018-05-10T14:09:37Z

@lbergelson Is this branch good to go?

lbergelson

@jamesemery Looks like there are a few leftover bits from the short era that need to be updated

lbergelson · 2018-05-10T15:42:18Z

src/main/java/org/broadinstitute/hellbender/utils/read/markduplicates/sparkrecords/Pair.java

    @Override
-    public void setX(final short x) { this.x = x; }
+    public void setX(final int x) { this.x = (short)x; }


you forgot to remove the casts to short when you updated this to int

lbergelson · 2018-05-10T15:42:57Z

...e/hellbender/utils/read/markduplicates/AbstractOpticalDuplicateFinderCommandLineProgram.java

@@ -24,7 +25,7 @@
    // Needed for testing
    public void setupOpticalDuplicateFinder() {
        this.opticalDuplicateFinder = new OpticalDuplicateFinder(opticalDuplicatesArgumentCollection.READ_NAME_REGEX,
-            opticalDuplicatesArgumentCollection.OPTICAL_DUPLICATE_PIXEL_DISTANCE, logger);
+            opticalDuplicatesArgumentCollection.OPTICAL_DUPLICATE_PIXEL_DISTANCE,null);//TODO logger firgure out, logger);


this still has this typo

@lbergelson right, so i don't know the answer to this. Switiching over to the picard version of the optical duplicate finder means that we are no longer processing optical duplicates with our logger but the picard one, which happens to not be serializable. Should I just give it a null logger and call it a day then?

null logger is fine for now . Could you open a ticket to enable us to pass a logger into it so we remember to look at it again?

lbergelson · 2018-05-10T15:51:52Z

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


    @Override
-    public void setY(final short y) { this.y = y; }
+    public void setY(final int y) { this.y = (short) y; }


leftover cast to short here

lbergelson · 2018-05-10T15:52:33Z

src/main/java/org/broadinstitute/hellbender/utils/read/markduplicates/sparkrecords/Pair.java

@@ -34,7 +34,7 @@
    private final int score;

    // Information used to detect optical dupes
-    private transient short readGroup = -1;
+    private short readGroupIndex = -1;


could you add a line break between this and the transient ones?

lbergelson · 2018-05-10T15:52:47Z

src/main/java/org/broadinstitute/hellbender/utils/read/markduplicates/sparkrecords/Pair.java

@@ -92,7 +92,7 @@ private Pair(Kryo kryo, Input input){
        super(input.readInt(true), input.readString());

        // Information used to detect optical dupes
-        readGroup = -1;
+        readGroupIndex = -1;


this line should be removed?

lbergelson · 2018-05-10T15:53:14Z

src/main/java/org/broadinstitute/hellbender/utils/read/markduplicates/sparkrecords/Pair.java

    @Override
-    public void setY(final short y) { this.y = y; }
+    public void setY(final int y) { this.y = (short)y; }


lbergelson · 2018-05-11T16:56:35Z

@jamesemery We have test failures again https://storage.googleapis.com/hellbender-test-logs/build_reports/master_19079.2/tests/test/index.html

…ances in that test should not have been considered close...

jamesemery · 2018-05-11T18:10:36Z

@lbergelson Yeah, so the test was broken because it was assuming that the pairedEnds y value was getting truncated. Now that test is running on two reads that are ACTUALLY close enough for comparison.

…ends objects

lbergelson · 2018-05-14T16:50:48Z

...roadinstitute/hellbender/tools/spark/transforms/markduplicates/MarkDuplicatesSparkUtils.java

-                final Short readGroup = headerReadGroupIndexMap.getValue().get(firstRead.getValue().getReadGroup());
+                final Pair pair = MarkDuplicatesSparkRecord.newPair(firstRead, secondRead.getValue(), header, secondRead.getIndex(), scoringStrategy);
+                // Validate and add the read group to the pair
+                final Short readGroup = headerReadGroupIndexMap.getValue().get(firstRead.getReadGroup());


could we add 2 simple tests that show that it explodes with the right errors on each of these cases

lbergelson

looks good to me, merge when tests are green

lbergelson · 2018-05-14T19:56:22Z

https://storage.googleapis.com/hellbender-test-logs/build_reports/master_19134.2/tests/test/index.html

…pringing up in new places

jamesemery requested a review from lbergelson May 8, 2018 19:10

lbergelson reviewed May 9, 2018

View reviewed changes

jamesemery assigned lbergelson May 9, 2018

lbergelson reviewed May 9, 2018

View reviewed changes

lbergelson requested changes May 9, 2018

View reviewed changes

lbergelson assigned jamesemery and unassigned lbergelson May 9, 2018

jamesemery assigned lbergelson and unassigned jamesemery May 9, 2018

jamesemery added 6 commits May 10, 2018 10:08

First stage is admitting your grief

5909dfb

Switched to be in step with picard duplicate marking

6daaf1f

Added tests for different ReadGroup behavior in the optical duplicate…

e6da313

… marking code

responded to comments

694b586

fixed the broken tests

2be518e

Returned to using ints in lieu of shorts for physical locations

8971ef6

jamesemery force-pushed the je_opticalDuplicateFinding branch from abc1ca6 to 8971ef6 Compare May 10, 2018 14:09

fixed an error that slipped through the cracks

0c7ac20

lbergelson requested changes May 10, 2018

View reviewed changes

Can an era known for short really be called an era?

193a644

updating the physical locations to use ints illuminated that the dist…

7988acb

…ances in that test should not have been considered close...

Added better error messages for validating the read groups for paired…

e950432

…ends objects

lbergelson reviewed May 14, 2018

View reviewed changes

added tests for the error states

464c723

lbergelson approved these changes May 14, 2018

View reviewed changes

jamesemery added 2 commits May 14, 2018 16:50

Its like plugging leakes in the dam with my fingers, they just keep s…

83c1d0b

…pringing up in new places

no seriously this time

8b13862

lbergelson approved these changes May 14, 2018

View reviewed changes

jamesemery merged commit 6855752 into master May 15, 2018

davidbenjamin deleted the je_opticalDuplicateFinding branch February 24, 2021 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated MarkDuplicates code to rely on the Picard OpticalDuplicatesFinder #4750

Updated MarkDuplicates code to rely on the Picard OpticalDuplicatesFinder #4750

jamesemery commented May 8, 2018

lbergelson May 9, 2018

jamesemery May 9, 2018

lbergelson May 9, 2018

jamesemery May 9, 2018

lbergelson May 9, 2018

jamesemery May 9, 2018

lbergelson May 9, 2018

jamesemery May 9, 2018

lbergelson left a comment

lbergelson May 9, 2018

lbergelson May 9, 2018

lbergelson May 9, 2018

lbergelson May 9, 2018

jamesemery May 9, 2018

lbergelson May 9, 2018

jamesemery May 9, 2018

jamesemery commented May 9, 2018

codecov-io commented May 9, 2018 •

edited

Loading

jamesemery commented May 10, 2018

lbergelson left a comment

lbergelson May 10, 2018

lbergelson May 10, 2018

jamesemery May 10, 2018

lbergelson May 10, 2018

lbergelson May 10, 2018

lbergelson May 10, 2018

lbergelson May 10, 2018

lbergelson May 10, 2018

lbergelson commented May 11, 2018

jamesemery commented May 11, 2018 •

edited

Loading

lbergelson May 14, 2018

lbergelson left a comment

lbergelson commented May 14, 2018

Updated MarkDuplicates code to rely on the Picard OpticalDuplicatesFinder #4750

Updated MarkDuplicates code to rely on the Picard OpticalDuplicatesFinder #4750

Conversation

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

lbergelson 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

jamesemery commented May 9, 2018

codecov-io commented May 9, 2018 • edited Loading

Codecov Report

jamesemery commented May 10, 2018

lbergelson 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

lbergelson commented May 11, 2018

jamesemery commented May 11, 2018 • edited Loading

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

lbergelson commented May 14, 2018

codecov-io commented May 9, 2018 •

edited

Loading

jamesemery commented May 11, 2018 •

edited

Loading