Added a distinction between PCR orientation and Optical Duplicates orientation in MarkDuplicatesSpark #4752

jamesemery · 2018-05-09T16:41:31Z

Blocked by #4732

codecov-io · 2018-05-14T21:21:31Z

Codecov Report

Merging #4752 into master will increase coverage by 0.05%.
The diff coverage is 93.333%.

@@              Coverage Diff               @@
##              master     #4752      +/-   ##
==============================================
+ Coverage     80.097%   80.147%   +0.05%     
- Complexity     17402     17558     +156     
==============================================
  Files           1080      1082       +2     
  Lines          63067     63387     +320     
  Branches       10174     10238      +64     
==============================================
+ Hits           50515     50803     +288     
- Misses          8566      8582      +16     
- Partials        3986      4002      +16

Impacted Files	Coverage Δ	Complexity Δ
...s/read/markduplicates/sparkrecords/PairedEnds.java	`100% <ø> (ø)`	`1 <0> (ø)`	⬇️
...transforms/markduplicates/MarkDuplicatesSpark.java	`95.918% <ø> (ø)`	`16 <0> (ø)`	⬇️
...ils/read/markduplicates/sparkrecords/Fragment.java	`100% <100%> (ø)`	`11 <2> (+2)`	⬆️
...r/utils/read/markduplicates/sparkrecords/Pair.java	`96.667% <100%> (+3.333%)`	`29 <12> (+6)`	⬆️
...ead/markduplicates/sparkrecords/EmptyFragment.java	`88.889% <100%> (+0.654%)`	`9 <2> (+2)`	⬆️
...forms/markduplicates/MarkDuplicatesSparkUtils.java	`89.27% <75%> (-0.73%)`	`69 <2> (ø)`
...utils/smithwaterman/SmithWatermanIntelAligner.java	`50% <0%> (-30%)`	`1% <0%> (-2%)`
...ellbender/utils/test/CommandLineProgramTester.java	`91.667% <0%> (-3.571%)`	`11% <0%> (+2%)`
...itute/hellbender/engine/spark/GATKRegistrator.java	`100% <0%> (ø)`	`6% <0%> (+3%)`	⬆️
...titute/hellbender/engine/TwoPassVariantWalker.java	`95% <0%> (ø)`	`4% <0%> (?)`
... and 5 more

…een orientation for optical duplicats and orientation for PCR duplicates

jamesemery · 2018-05-15T18:09:23Z

@lbergelson This is rebased on master and theoretically working representing that bugfix now when you get the chance to take a look.

lbergelson

I have a few aesthetic comments, docs, and a request for an additional test.

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

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

-                (((long)((PairedEnds)record).getUnclippedStartPosition()) << 32 |
-                        ((PairedEnds)record).getFirstRefIndex() << 16 );
-        //| ((PairedEnds)pe).getLibraryIndex())).values();
+                ((((long)((PairedEnds)record).getUnclippedStartPosition()) << 32) |


I would remove this redundant casting, by making the type of the record argument PairedEnds and then casting when you call instead of here.

lbergelson · 2018-05-17T18:57:06Z

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

        if (R1R && R2R) {
            return ReadEnds.RR;
        }
        if (R1R) {
-            return ReadEnds.RF; //at this point we know for sure R2R is false
+            return (optical&&wasFlipped)? ReadEnds.FR : ReadEnds.RF; //at this point we know for sure R2R is false


can we get some spaces here

lbergelson · 2018-05-17T18:57:11Z

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

        }
        if (R2R) {
-            return ReadEnds.FR; //at this point we know for sure R1R is false
+            return (optical&&wasFlipped)? ReadEnds.RF :ReadEnds.FR; //at this point we know for sure R1R is false


lbergelson · 2018-05-17T19:04:56Z

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

+    }
+
+    @Override
+    public byte getPCROrientation() {


maybe rename to getOrientationForPCRDuplicates? It makes it sound like it's somehow the orientation of the PCR which doesn't mean anything to me.

This method should get some javadoc too explain why it's different than optical.

lbergelson · 2018-05-17T19:06:46Z

...in/java/org/broadinstitute/hellbender/utils/read/markduplicates/sparkrecords/PairedEnds.java

    public abstract int getScore();
-
+    public abstract boolean isR1R();


should we rename this? it's totally unintuitive, we could just expand to isRead1ReverseStrand which is still kind of nasty, but at least someone could figure it out

lbergelson · 2018-05-17T19:07:30Z

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

@@ -171,14 +166,23 @@ public String toString() {
     * Returns one of {@link ReadEnds#RR}, {@link ReadEnds#RF}, {@link ReadEnds#FR}, {@link ReadEnds#FF}
     */
    public byte getOrientationForOpticalDuplicates() {


I think we want unit tests for these two methods that assert the truth table for the two different type of orientations

jamesemery · 2018-05-18T16:43:49Z

@lbergelson Responded to your comments

lbergelson

👍

jamesemery requested a review from lbergelson May 9, 2018 16:41

jamesemery assigned lbergelson May 9, 2018

jamesemery force-pushed the je_MDOrientationWork branch from 2120c70 to 918645b Compare May 14, 2018 17:42

Fixed an issue where gatkDuplicateMarking was not distinguishing betw…

a24be24

…een orientation for optical duplicats and orientation for PCR duplicates

jamesemery force-pushed the je_MDOrientationWork branch from 8a0d340 to a24be24 Compare May 15, 2018 17:36

lbergelson requested changes May 17, 2018

View reviewed changes

lbergelson assigned jamesemery and unassigned lbergelson May 17, 2018

responded to comments

8587996

jamesemery assigned lbergelson and unassigned jamesemery May 18, 2018

lbergelson approved these changes May 18, 2018

View reviewed changes

lbergelson merged commit 767b420 into master May 18, 2018

lbergelson deleted the je_MDOrientationWork branch May 18, 2018 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a distinction between PCR orientation and Optical Duplicates orientation in MarkDuplicatesSpark #4752

Added a distinction between PCR orientation and Optical Duplicates orientation in MarkDuplicatesSpark #4752

jamesemery commented May 9, 2018 •

edited

Loading

codecov-io commented May 14, 2018 •

edited

Loading

jamesemery commented May 15, 2018

lbergelson left a comment

lbergelson May 17, 2018

jamesemery May 18, 2018

lbergelson May 17, 2018

jamesemery May 18, 2018

lbergelson May 17, 2018

jamesemery May 18, 2018

lbergelson May 17, 2018

lbergelson May 17, 2018

jamesemery May 18, 2018

lbergelson May 17, 2018

jamesemery May 18, 2018

lbergelson May 17, 2018

jamesemery commented May 18, 2018

lbergelson left a comment

		public abstract int getScore();

		public abstract boolean isR1R();

Added a distinction between PCR orientation and Optical Duplicates orientation in MarkDuplicatesSpark #4752

Added a distinction between PCR orientation and Optical Duplicates orientation in MarkDuplicatesSpark #4752

Conversation

jamesemery commented May 9, 2018 • edited Loading

codecov-io commented May 14, 2018 • edited Loading

Codecov Report

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

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 18, 2018

lbergelson left a comment

Choose a reason for hiding this comment

jamesemery commented May 9, 2018 •

edited

Loading

codecov-io commented May 14, 2018 •

edited

Loading