Switched MarkDuplicatesSpark tile parsing code to use shorts in order to match picard #5165
Conversation
There was a problem hiding this comment.
@jamesemery A few comments and a question. Change looks good, just some wonky things you forgot in your test code.
| public void test() { | ||
| final ArgumentsBuilder args = new ArgumentsBuilder(); | ||
| args.addOutput(createTempFile("output","bam")); | ||
| args.addInput(new File("/Users/emeryj/hellbender/gatk/compareDuplicates/picarddiffs.sam")); |
There was a problem hiding this comment.
This path is local to your machine.
There was a problem hiding this comment.
whoops, that was a test file from my evaluation, ill get rid of that
| @@ -189,6 +189,14 @@ public void testSupplementaryReadUnmappedMate() { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void test() { | |||
There was a problem hiding this comment.
test isn't exactly a clear name
| @@ -251,7 +254,7 @@ public void testOpticalDuplicatesTheSameReadGroup() { | |||
| public void testOpticalDuplicatesAndPCRDuplicatesOrientationDifference() { | |||
| final AbstractMarkDuplicatesTester tester = getTester(); | |||
| tester.setExpectedOpticalDuplicate(0); | |||
| tester.addMatePair("RUNID:7:1203:2886:82292", 19, 19, 485253, 486253, false, false, true, true, "101M", "101M", true, false, false, false, false, DEFAULT_BASE_QUALITY, "1"); // duplicate | |||
| tester.addMatePair("RUNID:7:1203:2886:16838", 19, 19, 485253, 486253, false, false, true, true, "101M", "101M", true, false, false, false, false, DEFAULT_BASE_QUALITY, "1"); // duplicate | |||
There was a problem hiding this comment.
Why did this one need to change? If it resolves differently now, how were both we and picard passing before? Or is this a gatk only test?
There was a problem hiding this comment.
So this test is different now because the tiebreaking has changed, specifically because of integer overflowing in the Y coordinate of this read. The point of this test is about orientation and optical duplicates (which it wasn't doing a good job of testing before anyway because it was failing for a different reason). The tiebreaking change caused a different read to be marked as duplicate. I could have reversed the value but it seems better to correct the test as now its actually testing what it purports to. Also, yes this does not exist in picard.
There was a problem hiding this comment.
👍 Thanks for the explanation.
| @@ -37,13 +37,13 @@ public TransientFieldPhysicalLocation(int partitionIndex, String name) { | |||
| public int getX() { return this.x; } | |||
|
|
|||
| @Override | |||
| public void setX(final int x) { this.x = x; } | |||
There was a problem hiding this comment.
I would add a comment saying that in practice picard always uses short here even though the api is int.
|
@lbergelson responded to comments |
…ushed the shared test down
No description provided.