Clarified behavior of MarkDuplicatesSpark when given multiple input bams #5901
Conversation
|
This also raises another question. There is no particularly strong reason to restrict multi-bam mode to inputs of a particular sort order. We could happily treat multiple arbitrarily unsorted inputs as just that and sort them as the first step. Perhaps thats better than restricting them all to only work at all in the best case? Let me know your thoughts @droazen. |
Codecov Report
@@ Coverage Diff @@
## master #5901 +/- ##
==============================================
+ Coverage 86.838% 86.84% +0.003%
- Complexity 32325 32327 +2
==============================================
Files 1991 1991
Lines 149341 149347 +6
Branches 16483 16482 -1
==============================================
+ Hits 129684 129693 +9
+ Misses 13647 13646 -1
+ Partials 6010 6008 -2
|
There was a problem hiding this comment.
Minor comments only -- merge after addressing them @jamesemery
| @@ -47,6 +47,7 @@ | |||
| * <li>Due to MarkDuplicatesSpark queryname-sorting coordinate-sorted inputs internally at the start, the tool produces identical results regardless of the input sort-order. That is, it will flag duplicates sets that include secondary, and supplementary and unmapped mate records no matter the sort-order of the input. This differs from how Picard MarkDuplicates behaves given the differently sorted inputs. </li> | |||
| * <li>Collecting duplicate metrics slows down performance and thus the metrics collection is optional and must be specified for the Spark version of the tool with '-M'. It is possible to collect the metrics with the standalone Picard tool <a href='https://software.broadinstitute.org/gatk/documentation/tooldocs/current/picard_sam_markduplicates_EstimateLibraryComplexity.php'>EstimateLibraryComplexity</a>.</li> | |||
| * <li>MarkDuplicatesSpark is optimized to run locally on a single machine by leveraging core parallelism that MarkDuplicates and SortSam cannot. It will typically run faster than MarkDuplicates and SortSam by a factor of 15% over the same data at 2 cores and will scale linearly to upwards of 16 cores. This means MarkDuplicatesSpark, even without access to a Spark cluster, is faster than MarkDuplicates.</li> | |||
| * <li>MarkDuplicatesSpark can be run with multiple input bams. If this is the case all of the inputs must be querygroup ordered or queryname sorted.</li> | |||
There was a problem hiding this comment.
So a mix of querygroup-sorted and queryname-sorted inputs is ok? Or do they all have to be querygroup-sorted?
There was a problem hiding this comment.
Also: briefly explain the difference between the two orderings here (since it's not necessarily widely known).
| // Check if we are using multiple inputs that the headers are all in the correct querygrouped ordering | ||
| final SAMFileHeader header = getHeaderForReads(); | ||
|
|
||
| // Check if we are using multiple inputs that the headers are all in the correct querygrouped ordering, if so set the aggregate header to reflect this | ||
| Map<String, SAMFileHeader> headerMap = getReadSouceHeaderMap(); |
| @@ -288,12 +289,15 @@ public int getPartition(Object key) { | |||
|
|
|||
| @Override | |||
| protected void runTool(final JavaSparkContext ctx) { | |||
| // Check if we are using multiple inputs that the headers are all in the correct querygrouped ordering | |||
| final SAMFileHeader header = getHeaderForReads(); | |||
There was a problem hiding this comment.
This gets a merged header -- what will the underlying SamFileHeaderMerger do in the case where the sort order does not agree across headers? Will it throw in that case?
There was a problem hiding this comment.
Also, rename header to mergedHeader
| Map<String, SAMFileHeader> headerMap = getReadSouceHeaderMap(); | ||
| if (headerMap.size() > 1) { | ||
| headerMap.entrySet().stream().forEach(h -> {if(!ReadUtils.isReadNameGroupedBam(h.getValue())) { | ||
| throw new UserException("Multiple inputs to MarkDuplicatesSpark detected but input "+h.getKey()+" was sorted in "+h.getValue().getSortOrder()+" order"); | ||
| throw new UserException("Multiple inputs to MarkDuplicatesSpark detected. MarkDuplicatesSpark requires all inputs be queryname sorted or querygrouped for multi-input processing but input "+h.getKey()+" was sorted in "+h.getValue().getSortOrder()+" order"); |
There was a problem hiding this comment.
be -> to be
querygrouped -> querygroup-sorted
| }}); | ||
| header.setGroupOrder(SAMFileHeader.GroupOrder.query); |
There was a problem hiding this comment.
Doesn't SAMFileHeader.GroupOrder.query correspond to querygrouped ordering? Is it ok to set this here if not all inputs are necessarily querygrouped?
There was a problem hiding this comment.
Query grouped bams are a superset of queryname sorted bams. So long as all the inputs are querygrouped (and I suppose also that they don't have overlapping readnames...) the inputs are valid.
Also improved the sorting behavior if given a mix of queryname sorted and query-grouped bams.
@bhanugandham