-
Notifications
You must be signed in to change notification settings - Fork 589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(SV) more alignment edge cases #5044
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5044 +/- ##
===============================================
- Coverage 86.382% 80.371% -6.011%
+ Complexity 28674 27677 -997
===============================================
Files 1785 1785
Lines 132713 133530 +817
Branches 14766 15004 +238
===============================================
- Hits 114640 107319 -7321
- Misses 12746 21030 +8284
+ Partials 5327 5181 -146
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple of refactoring suggestions and one implementation question but otherwise this looks ok. Make sure you run it on the sample that was failing to test it out.
@@ -571,13 +572,30 @@ private static double computeTigExplainQualOfOneConfiguration(final List<Alignme | |||
return bestConfigurations.stream() | |||
.map(mappings -> splitGaps(mappings, false)) | |||
.map(mappings -> removeNonUniqueMappings(mappings, ALIGNMENT_MQ_THRESHOLD, ALIGNMENT_LOW_READ_UNIQUENESS_THRESHOLD)) | |||
.filter(mappings -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be cleaner like this?
.filter(mappings -> mappings.goodMappings.size != 2 || ! simpleChimeraWithStichableAlignments(mappings.goodMappings.get(0), mappings.goodMappings.get(1))))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
.map(mappings -> createContigGivenClassifiedAlignments(contigName, contigSeq, mappings, true)) | ||
.sorted(getConfigurationComparator()) | ||
.iterator(); | ||
} else { | ||
final GoodAndBadMappings intermediate = splitGaps(bestConfigurations.get(0), false); | ||
final GoodAndBadMappings result = removeNonUniqueMappings(intermediate, ALIGNMENT_MQ_THRESHOLD, ALIGNMENT_LOW_READ_UNIQUENESS_THRESHOLD); | ||
return Collections.singletonList(createContigGivenClassifiedAlignments(contigName, contigSeq, result, false)).iterator(); | ||
if (result.goodMappings.size() != 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do the refactoring suggested above you could make a little method for the if clause and only write it once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -853,11 +863,14 @@ private static void removeDueToShortReadSpan(final List<AlignmentInterval> selec | |||
} | |||
} | |||
|
|||
@VisibleForTesting | |||
static final int TEMPORARY_SELF_POINTING_SV_INTERVAL_CONTIG = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this. It doesn't seem like you ever refer to that except when putting things into the max overlap map below. Why create these invalid intervals? Also, I think that our contig numbering scheme for SVInterval
starts at zero so these fake intervals will look like real intervals. I think I'd much rather keep them the old way in Tuple2<Integer, Integer>
s than jam them into invalid interval objects.
* See the funny alignment signature described in ticket 4951 on GATK github | ||
* @param intervalOne assumed to start no later than {@code intervalTwo} on the read | ||
* @param intervalTwo assumed to start no earlier than {@code intervalOne} on the read | ||
* @return if the two given intervals can be stitched together |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"true if"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
All 4 trios pass without problems, by taking in the assembly bams from Ted's branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for making those changes.
👍
* ticket 4951 * two neighboring gapped alignments, after gap split, generate unsorted alignments * also a new utility method in SVInterval
1266952
to
a4b63ce
Compare
Thanks for the quick review! |
fixes #4951
And another alignment edge case illustrated below (SAM records)