Validate SVCallRecord coordinates #7714
Conversation
…terval collapsing
| Utils.nonNull(dictionary); | ||
| validatePosition(contigA, positionA, dictionary); | ||
| validatePosition(contigB, positionB, dictionary); | ||
| Utils.validateArg(IntervalUtils.compareLocatables(getPositionAInterval(), getPositionBInterval(), dictionary) <= 0, |
There was a problem hiding this comment.
I forget -- do we do start == end for insertions in SVCallRecords? Just checking whether this needs to be <= or not. If we do, you might want to add a quick unit test condition to your new test in SVCallRecordUnitTest where start == end with a comment.
There was a problem hiding this comment.
Yes start==end should be correct. I've added a test with some valid coordinates to make sure they pass.
| if (!getPaddedRecordInterval(a.getContigA(), a.getPositionA(), a.getPositionB()) | ||
| .overlaps(getPaddedRecordInterval(b.getContigA(), b.getPositionA(), b.getPositionB()))) return false; | ||
| final SimpleInterval intervalA = getPaddedRecordInterval(a.getContigA(), a.getPositionA(), a.getPositionB()); | ||
| Utils.nonNull(intervalA, "Invalid interval " + new SimpleInterval(a.getContigA(), a.getPositionA(), |
There was a problem hiding this comment.
It looks like the getPaddedRecordInterval method returns null in the odd cases where the padded start is greater than the contig end or the padded stop is less than 1. Am I missing other cases? If not, aren't we already validating the coordinates when we construct the SVCallRecord object? I guess this assertion is there to be thorough -- maybe a short comment here explaining what the possible error condition is would be helpful to code readers?
There was a problem hiding this comment.
Yes you're right I added checks for valid coordinates so this wouldn't happen, and this is here simply for safety. I've added a comment about it.
| return new Object[][]{ | ||
| {"chr1", 0, "chr1", 248956422}, | ||
| {"chr1", 1, "chr1", 248956423}, | ||
| {"chr1", 1, "chr1", 248956423}, |
There was a problem hiding this comment.
As I mentioned above maybe add a case where start == end? (I guess you'd have to change this test to handle positive examples as well..)
| Lists.newArrayList(Allele.REF_N, Allele.SV_SIMPLE_DEL), | ||
| Collections.emptyList(), Collections.emptyMap(), dictionary); | ||
| defragmenter.areClusterable(call1, call2); | ||
| Assert.fail("Expected exception not thrown"); |
There was a problem hiding this comment.
Not a big deal, but it might be a little more explicit to catch the exception and assert it's of the right type, etc. This test pattern can mask something going wrong in the error handling code (ie it would be pass if an NPE came back).
There was a problem hiding this comment.
Actually I realized that the exception thrown in these tests was actually happening when creating the invalid SVCallRecord, not during areClusterable(). There's no way around this (by design), so I've deleted these tests.
-Adds start/end coordinate validation to
SVCallRecord, checking contigs and positions against the sequence dictionary and their ordering.-Adds some checks for invalid coordinates in places where
SimpleInterval.expandWithinContig()can potentially returnnull.-Addresses an issue where inter-chromosomal records' end positions were incorrectly disallowed from preceding the start position during record collapsing (despite being on a different contig).
-Deletes unused
SVCallRecordWithEvidenceIncludes regression tests.