Added a few of the remaining useful features to MarkDuplicatesSpark and associated tests #5377

jamesemery · 2018-10-30T21:13:52Z

Note: This branch is still blocked on several changes in Picard (broadinstitute/picard#1236, and possibly broadinstitute/picard#1245), once those are resolved and released then this branch should hopefully get the stamp of approval from @takutosato.

Added optical/library duplicate marking of reads (note: this does not include library tagging)
Added the ability to remove reads from the output bam based on their duplicate marking status

Resolves #4675
Resolves #5377

lbergelson

@jamesemery Back to you. Looks good modulo typos and some random stuff.

lbergelson · 2018-11-02T18:02:34Z

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

+            mutex = {MarkDuplicatesSparkArgumentCollection.DUPLICATE_TAGGING_POLICY_LONG_NAME, MarkDuplicatesSparkArgumentCollection.REMOVE_SEQUENCING_DUPLICATE_READS}, optional = true)
+    public boolean removeAllDuplicates = false;
+
+    @Argument(fullName = MarkDuplicatesSparkArgumentCollection.REMOVE_SEQUENCING_DUPLICATE_READS, doc = "If true do not write duplicates to the output file instead of writing them with appropriate flags set.",


This is confusing because it has the same comment as removeAllDuplicates.

lbergelson · 2018-11-02T18:18:08Z

src/main/java/org/broadinstitute/hellbender/engine/spark/GATKRegistrator.java

@@ -93,5 +94,6 @@ private void registerGATKClasses(Kryo kryo) {
        kryo.register(ReadsKey.class, new FieldSerializer(kryo, ReadsKey.class));
        kryo.register(ReadsKey.KeyForFragment.class, new FieldSerializer(kryo, ReadsKey.KeyForFragment.class));
        kryo.register(ReadsKey.KeyForPair.class, new FieldSerializer(kryo, ReadsKey.KeyForPair.class));
+        kryo.register(SerializableOpticalDuplicatesFinder.class, new FieldSerializer(kryo, SerializableOpticalDuplicatesFinder.class));


Weren't we getting rid of this class? Should this be replaces with a registration of OpticalDuplicateFinder?

lbergelson · 2018-11-02T18:18:45Z

...a/org/broadinstitute/hellbender/tools/spark/pipelines/BwaAndMarkDuplicatesPipelineSpark.java

@@ -68,7 +68,7 @@ protected void runTool(final JavaSparkContext ctx) {
        try (final BwaSparkEngine bwaEngine = new BwaSparkEngine(ctx, referenceArguments.getReferenceFileName(), bwaArgs.indexImageFile, getHeaderForReads(), getReferenceSequenceDictionary())) {
            final ReadFilter filter = makeReadFilter(bwaEngine.getHeader());
            final JavaRDD<GATKRead> alignedReads = bwaEngine.alignPaired(getUnfilteredReads()).filter(filter::test);
-            final JavaRDD<GATKRead> markedReads = MarkDuplicatesSpark.mark(alignedReads, bwaEngine.getHeader(), markDuplicatesSparkArgumentCollection.duplicatesScoringStrategy, new SerializableOpticalDuplicatesFinder(), getRecommendedNumReducers(), markDuplicatesSparkArgumentCollection.dontMarkUnmappedMates);
+            final JavaRDD<GATKRead> markedReads = MarkDuplicatesSpark.mark(alignedReads, bwaEngine.getHeader(), new SerializableOpticalDuplicatesFinder(), markDuplicatesSparkArgumentCollection, getRecommendedNumReducers());


SerializableOpticalDuplicateFinder -> OpticalDuplicateFinder?

lbergelson · 2018-11-02T18:18:54Z

src/main/java/org/broadinstitute/hellbender/tools/spark/pipelines/ReadsPipelineSpark.java

@@ -174,7 +174,7 @@ protected void runTool(final JavaSparkContext ctx) {
            header = getHeaderForReads();
        }

-        final JavaRDD<GATKRead> markedReads = MarkDuplicatesSpark.mark(alignedReads, header, markDuplicatesSparkArgumentCollection.duplicatesScoringStrategy, new SerializableOpticalDuplicatesFinder(), getRecommendedNumReducers(), markDuplicatesSparkArgumentCollection.dontMarkUnmappedMates);
+        final JavaRDD<GATKRead> markedReads = MarkDuplicatesSpark.mark(alignedReads, header, new SerializableOpticalDuplicatesFinder(), markDuplicatesSparkArgumentCollection, getRecommendedNumReducers());


lbergelson · 2018-11-02T18:19:35Z

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

+    // Reads with this marker will be treated as non-duplicates always
+    public static int MARKDUPLICATES_NO_OPTICAL_MARKER = -1;
+    // Reads with this marker will be treated and marked as optical duplicates
+    public static int MARKDUPLICATES_OPPTICAL_DUPLICATE_MARKER = -2;


typo OPPTICAL

lbergelson · 2018-11-02T18:37:19Z

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

+        try {
+            metricsOutput.read(new FileReader(metricsFile));
+        } catch (final FileNotFoundException ex) {
+            System.err.println("Metrics file not found: " + ex);


Shouldn't this be a test failure?

lbergelson · 2018-11-02T18:44:34Z

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

+        } catch (final FileNotFoundException ex) {
+            System.err.println("Metrics file not found: " + ex);
+        }
+        final List<GATKDuplicationMetrics> nonEmptyMetrics = metricsOutput.getMetrics().stream().filter(


this is a big block of complicated duplicated code, could you extract it to a method instead?

lbergelson · 2018-11-02T18:49:05Z

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

+            Assert.assertEquals(observedMetrics.READ_PAIR_OPTICAL_DUPLICATES, expectedList.get(5));
+            Assert.assertEquals(observedMetrics.PERCENT_DUPLICATION, expectedList.get(6));
+
+            //Note: IntelliJ does not like it when a parameter for a test is null (can't print it and skips the test)


I'm not sure this comment is true? ImmutableList.of doesn't like nulls but I didn't think intellij had any trouble with it.

Not my comment, was originally adams

lbergelson · 2018-11-02T18:50:23Z

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

+            List<?> expectedList = metricsExpected.get(observedMetrics.LIBRARY);
+            expectedOpticalDuplicatesGroups += (Long) expectedList.get(5);
+        }
+        // NOTE: this test will fail if we add a more comprehensive example set with optical duplicates containing secondary/supplementary reads


this makes it sound like there's a bug in the code, maybe make it clear that that it's the test that can't handle a more complicated scenerio?

lbergelson · 2018-11-02T18:52:05Z

src/main/java/org/broadinstitute/hellbender/tools/validation/CompareReads.java

+        oneLineSummary = "Compares the base qualities of two SAM/BAM/CRAM files",
+        programGroup = DiagnosticsAndQCProgramGroup.class
+)
+public class CompareReads extends GATKTool {


Did you mean to add this tool here? I don't see any tests for it.

This should either be removed or tested.

…nd associated tests

lbergelson

@jamesemery One comment I somehow missed in my previous review, can we push compare reads to another branch or add tests?

lbergelson · 2018-11-16T22:36:27Z

build.gradle

@@ -58,7 +58,7 @@ repositories {

 final requiredJavaVersion = "8"
 final htsjdkVersion = System.getProperty('htsjdk.version','2.16.1')
-final picardVersion = System.getProperty('picard.version','2.18.15')
+final picardVersion = System.getProperty('picard.version','2.18.14-22-ge9e24bf-SNAPSHOT')


there's a conflict here now

lbergelson · 2018-12-03T22:13:44Z

src/main/java/org/broadinstitute/hellbender/tools/validation/CompareReads.java

+        oneLineSummary = "Compares the base qualities of two SAM/BAM/CRAM files",
+        programGroup = DiagnosticsAndQCProgramGroup.class
+)
+public class CompareReads extends GATKTool {


This should either be removed or tested.

jamesemery · 2018-12-10T16:25:35Z

@lbergelson I removed compareReads. Can you give me another review? I would like to get this branch in in the shortish term.

lbergelson

👍

jamesemery requested a review from lbergelson October 30, 2018 21:13

lbergelson requested changes Nov 2, 2018

View reviewed changes

lbergelson assigned jamesemery Nov 2, 2018

jamesemery added 4 commits November 28, 2018 15:47

Added a few of the remaining useful features to MarkDuplicatesSpark a…

c641252

…nd associated tests

rebuilding the image

14dadef

respnoded to comments without deleting the class

5306d50

reverting the changes to the picard version

891d2f0

jamesemery force-pushed the je_finalFeaturesForMarkDuplicatesTieout branch from 298349b to 891d2f0 Compare November 28, 2018 20:47

lbergelson requested changes Dec 3, 2018

View reviewed changes

removed compare reads

5b0d6c9

jamesemery assigned abaumann, jamesemery and lbergelson and unassigned jamesemery and abaumann Dec 10, 2018

lbergelson approved these changes Dec 11, 2018

View reviewed changes

lbergelson merged commit d2b7bb8 into master Dec 11, 2018

lbergelson deleted the je_finalFeaturesForMarkDuplicatesTieout branch December 11, 2018 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a few of the remaining useful features to MarkDuplicatesSpark and associated tests #5377

Added a few of the remaining useful features to MarkDuplicatesSpark and associated tests #5377

jamesemery commented Oct 30, 2018 •

edited

Loading

lbergelson left a comment

lbergelson Nov 2, 2018

lbergelson Nov 2, 2018

jamesemery Nov 5, 2018

lbergelson Nov 2, 2018

lbergelson Nov 2, 2018

lbergelson Nov 2, 2018

lbergelson Nov 2, 2018

lbergelson Nov 2, 2018

lbergelson Nov 2, 2018

jamesemery Nov 5, 2018

lbergelson Nov 2, 2018

lbergelson Nov 2, 2018

lbergelson Dec 3, 2018

lbergelson left a comment

lbergelson Nov 16, 2018

lbergelson Dec 3, 2018

jamesemery commented Dec 10, 2018

lbergelson left a comment

Added a few of the remaining useful features to MarkDuplicatesSpark and associated tests #5377

Added a few of the remaining useful features to MarkDuplicatesSpark and associated tests #5377

Conversation

jamesemery commented Oct 30, 2018 • edited Loading

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

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

jamesemery commented Dec 10, 2018

lbergelson left a comment

Choose a reason for hiding this comment

jamesemery commented Oct 30, 2018 •

edited

Loading