⭐ Must Read: broadinstitute/gatk/pull/

jamesemery · 2018-11-16T21:16:38Z

I'm not sure how much I like the changes to GATKSparkTool.

Resolves #5398

tomwhite · 2018-11-19T10:49:21Z

+        doNotMerge,
+        concatMerge,
+        mergeAndSort
+    }


The mergeAndSort policy is not used or implemented, so shouldn't be added yet. The walker version doesn't have such a policy as far as I can see, it effectively implements concatMerge, so just implementing the equivalent would be sufficient, no?

tomwhite · 2018-11-19T10:49:42Z

    }

+    /**
+     * Does this tool support multiple inputs? Tools that do should


Unfinished sentence.

tomwhite · 2018-11-19T10:50:26Z


-        // TODO: This if statement is a temporary hack until #959 gets resolved.
-        if (readInput.endsWith(".adam")) {
+        // TODO: This if statement is a temporary hack until #959 gets resolve


This comment doesn't apply to the next line - it should be moved to the code in getGatkReadJavaRDD.

tomwhite · 2018-11-19T10:53:46Z

+     * @return doNotMerge by default
+     */
+    public ReadInputMergingPolicy getReadInputMergingPolicy() {
+        return ReadInputMergingPolicy.doNotMerge;


Why not default to concatMerge and support multiple inputs for all Spark tools?

I debated this with @lbergelson. It seems like an undesirable behavior for tools that tailor their behavior to the header sort order to union the RDDs of multiple inputs potentially invalidating any assumptions about input ordering.

I'm in favor of tools opting in.

tomwhite · 2018-11-19T10:55:15Z

+        headers.addAll(readInputs.values());
+
+        SamFileHeaderMerger headerMerger = new SamFileHeaderMerger(identifySortOrder(headers), headers, true);
+        return headerMerger;


No need for headerMerger variable.

tomwhite · 2018-11-19T10:58:57Z

+     */
+    private SamFileHeaderMerger createHeaderMerger() {
+        List<SAMFileHeader> headers = new ArrayList<>(readInputs.size());
+        headers.addAll(readInputs.values());


Does this need to be a list? You are using a LinkedHashMap, so you know values() will be ordered correctly. SamFileHeaderMerger takes a Collection, and identifySortOrder could too.

tomwhite · 2018-11-19T11:00:00Z

+
+    @Test (expectedExceptions = UserException.class)
+    public void testAssertCorrectSortOrderMultipleBams() {
+        // This test asserts that the handling of two read pairs with the same start positions but on different in such a way


Comment wording is a bit unclear.

jamesemery · 2018-11-28T20:33:35Z

@tomwhite Can I get another round of review on this branch?

codecov-io · 2018-11-28T21:34:32Z

Codecov Report

Merging #5430 into master will decrease coverage by 0.003%.
The diff coverage is 86.25%.

@@               Coverage Diff               @@
##              master     #5430       +/-   ##
===============================================
- Coverage     87.085%   87.082%   -0.003%     
- Complexity     31222     31251       +29     
===============================================
  Files           1915      1915               
  Lines         144079    144178       +99     
  Branches       15891     15910       +19     
===============================================
+ Hits          125471    125553       +82     
- Misses         12837     12839        +2     
- Partials        5771      5786       +15

Impacted Files	Coverage Δ	Complexity Δ
...pelines/metrics/QualityScoreDistributionSpark.java	`95.238% <100%> (ø)`	`16 <0> (ø)`	⬇️
...transforms/markduplicates/MarkDuplicatesSpark.java	`94.872% <100%> (+0.427%)`	`38 <9> (+4)`	⬆️
...pipelines/metrics/CollectMultipleMetricsSpark.java	`92.593% <100%> (ø)`	`9 <0> (ø)`	⬇️
...k/pipelines/metrics/MetricsCollectorSparkTool.java	`75% <100%> (ø)`	`3 <0> (ø)`	⬇️
...ark/pipelines/metrics/MeanQualityByCycleSpark.java	`90.816% <100%> (ø)`	`11 <0> (ø)`	⬇️
...s/metrics/CollectBaseDistributionByCycleSpark.java	`87.037% <100%> (ø)`	`9 <0> (ø)`	⬇️
...stitute/hellbender/engine/spark/GATKSparkTool.java	`82.286% <76.923%> (-0.147%)`	`78 <9> (+10)`
.../pipelines/MarkDuplicatesSparkIntegrationTest.java	`91.144% <93.103%> (-0.262%)`	`42 <1> (+6)`
...hellbender/utils/runtime/RuntimeUtilsUnitTest.java	`87.5% <0%> (-12.5%)`	`4% <0%> (+1%)`
...te/hellbender/tools/funcotator/OutputRenderer.java	`92.857% <0%> (-7.143%)`	`4% <0%> (ø)`
... and 17 more

droazen · 2018-12-10T19:13:09Z

@tomwhite Looks like your latest comments have been addressed -- could you have another look at this branch when you get a chance?

lbergelson

@jamesemery Looks good to me. A few nitpicks but feel free to disregard them.

lbergelson · 2018-12-10T21:07:28Z

    protected String getReadSourceName(){
-        return readInput;
+        if (readInputs.size() > 1) {
+            throw new GATKException("Multiple ReadsDataSources specificed but a single source requested by the tool");


not ideal maybe but I don't know what else to do about this method... maybe change it to getReadSourceNames and return a list?

Right, I wanted to avoid returning a list because there are a bunch of places in the code where tools expect there to be only one read source kicking around and I didn't want to risk breaking something or having to uproot everything... I agree its pretty gross... Theoretically it shouldn't be a problem for most tools which don't accept multiple inputs anyway

I think we should change it to a list, or even get rid of it. All of the existing consumers except one use it to generate an output name for saving metrics (relatively easy to change); the other one (getRecommendedNumReducers) uses it to determine the number of reducers, which won't work correctly on multiple inputs with this implementation. I think getRecommendedNumReducers should be updated either way.

@cmnbroad Alright, I'll return a list. getRecommendedNumReducers has already been updated in this branch to sum over the all the read input files.

lbergelson · 2018-12-10T21:08:37Z

+     * @return doNotMerge by default
+     */
+    public ReadInputMergingPolicy getReadInputMergingPolicy() {
+        return ReadInputMergingPolicy.doNotMerge;


I'm in favor of tools opting in.

lbergelson · 2018-12-10T21:09:58Z

+    }
+    @VisibleForTesting
+    static SAMFileHeader.SortOrder identifySortOrder(final Collection<SAMFileHeader> headers){
+        final Set<SAMFileHeader.SortOrder> sortOrders = headers.stream().map(SAMFileHeader::getSortOrder).collect(Collectors.toSet());


Clever way to check this.

lbergelson · 2018-12-10T21:12:27Z

+     */
+    private SamFileHeaderMerger createHeaderMerger() {
+        return new SamFileHeaderMerger(identifySortOrder(readInputs.values()), readInputs.values(), true);
+    }


There should be a newline here:

Suggested change

}

}

lbergelson · 2018-12-10T21:13:07Z

+        readsHeader = createHeaderMerger().getMergedHeader();
+    }
+
+


too many newlines here...

James responded to Tom's review already.

… if they are queryname sorted

…ng test failures

jamesemery requested review from lbergelson and tomwhite November 16, 2018 21:16

tomwhite previously requested changes Nov 19, 2018

View reviewed changes

droazen assigned tomwhite Nov 30, 2018

lbergelson approved these changes Dec 10, 2018

View reviewed changes

lbergelson assigned jamesemery and unassigned tomwhite Dec 10, 2018

tomwhite approved these changes Dec 11, 2018

View reviewed changes

James added 7 commits January 2, 2019 15:21

some progress?

c123270

Added the ability for MarkDuplicatesSpark to take multiple input bams…

d2ce086

… if they are queryname sorted

responded to comments

27dd6c1

fixing a compiler warning

d6c47f2

Fixed a bug in the recommended number of reducers code that was causi…

7ed7a00

…ng test failures

resplonding to final comments

c4efaab

resovling the conflict of the orders

bf58fd3

jamesemery force-pushed the je_makeMarkDuplicatesSparkAcceptMultipleInputs branch from b558fb2 to bf58fd3 Compare January 2, 2019 20:36

James added 2 commits January 3, 2019 11:00

fixing the merge conflict failures

3a54fdb

responding to cnormans comment

73155c6

jamesemery merged commit 1fb980c into master Jan 3, 2019

jamesemery deleted the je_makeMarkDuplicatesSparkAcceptMultipleInputs branch January 3, 2019 21:03

Uh oh!

Conversation

jamesemery commented Nov 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesemery commented Nov 28, 2018

Uh oh!

codecov-io commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

droazen commented Dec 10, 2018

Uh oh!

lbergelson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmnbroad Jan 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Nov 28, 2018 •

edited

Loading

cmnbroad Jan 2, 2019 •

edited

Loading