Updated MarkDuplicates Scoring and Comparison code to reflect mismatches to picard #5023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #5023 +/- ##
==============================================
- Coverage 86.49% 86.488% -0.002%
- Complexity 29203 29270 +67
==============================================
Files 1814 1814
Lines 135364 135570 +206
Branches 15042 15069 +27
==============================================
+ Hits 117077 117252 +175
- Misses 12826 12848 +22
- Partials 5461 5470 +9
|
lbergelson
reviewed
Jul 18, 2018
| @@ -502,10 +502,7 @@ private PairedEndsCoordinateComparator() { } | |||
|
|
|||
| @Override | |||
| public int compare( TransientFieldPhysicalLocation first, TransientFieldPhysicalLocation second ) { | |||
There was a problem hiding this comment.
It's weird that this is called PairedEndsCoordiantComparator and it no longer compares PairedEnds or coordinates.
There was a problem hiding this comment.
The documentation for this is woefully out of date now.
| Utils.nonNull(read); | ||
| return scoring.applyAsInt(read); | ||
| // We max out at Short.MAX_VALUE / 2 to prevent overflow if we add two very high quality/length reads into pairs |
There was a problem hiding this comment.
the negative value if it fails vendor quality is strange, maybe mention that in a comment?
lbergelson
reviewed
Aug 13, 2018
| @@ -474,38 +480,43 @@ public static void saveMetricsRDD(final MetricsFile<GATKDuplicationMetrics, Doub | |||
| } | |||
|
|
|||
| /** | |||
| * Comparator for sorting Reads by coordinate. Note that a header is required in | |||
| * order to meaningfully compare contigs. | |||
| * Comparator for by their PhysicalLoccation attributes and strandedness. This comparator is intended to serve as a tiebreaker | |||
lbergelson
reviewed
Aug 13, 2018
| * It compares two pairedEnds objects by their first reads according to first their clipped start positions | ||
| * (they were matched together based on UnclippedStartOriginally), then the orientation of the strand, followed by | ||
| * the readname lexicographical order. | ||
| * Ordering is not the same as {@link htsjdk.samtools.SAMRecordCoordinateComparator}, except for the alignment position. |
There was a problem hiding this comment.
this comment is still out of date, it doesn't look at the alignment position anymore
jamesemery
force-pushed
the
je_updateDefaultScoringMechanism
branch
from
12f4487 to
8c978ad
Compare
lbergelson
reviewed
Aug 13, 2018
| @@ -480,23 +480,20 @@ public static void saveMetricsRDD(final MetricsFile<GATKDuplicationMetrics, Doub | |||
| } | |||
|
|
|||
| /** | |||
| * Comparator for by their PhysicalLoccation attributes and strandedness. This comparator is intended to serve as a tiebreaker | |||
| * Comparator for by their PhysicalLocation attributes and strandedness. This comparator is intended to serve as a tiebreaker | |||
jamesemery
force-pushed
the
je_updateDefaultScoringMechanism
branch
from
f258774 to
81bcdeb
Compare
… start position of mismatching reads. Additionally changed the score to be a double packed short value in order to better reflect picard scoring code.
jamesemery
force-pushed
the
je_updateDefaultScoringMechanism
branch
from
81bcdeb to
5db98c8
Compare
|
@lbergelson Removed the broken tests. The problem stemmed from markDuplicatesGATK being out of date. Since we are removing to remove it soon anyway i figured we would have the test only apply for markDuplicatesSpark |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After delving into picard and MarkDuplicates I have found two more sources of inconsistencies, first we were preserving the original mapped start position which we used for tie-breaking. This was unnecessary and caused problems at sites with very high depth. Additionally, our scoring code was out of step with the equivalent code in htsjdk which packed its results into shorts rather than ints.