Fixing zero-length interval bug in IntervalList.merge - #1318
Conversation
yfarjoun
left a comment
yfarjoun
left a comment
There was a problem hiding this comment.
please add tests around zero-length intervals.
| final String chrom = first.getContig(); | ||
| int start = first.getStart(); | ||
| int end = start; | ||
| int end = first.getEnd(); |
There was a problem hiding this comment.
what should happen when we merge a zero-length interval with a non-zerolength one after it?
so,
10-9 merged with 11-15 should remain those two intervals
10-9 merged with 10-15 should become one interval 10-15
10-9 merged with 9-15 should become one interval 9-15
There was a problem hiding this comment.
The first case, you mean it should become 10-15? Merge never leaves them separate.
There was a problem hiding this comment.
no...I think that it should not become 10-15....since the size of the first interval is 0 it shouldn't change the total number of bases covered...
There was a problem hiding this comment.
I think that zero length intervals need to either be dropped when merging, or ignored...normal rules of merging do not apply.
| final String chrom = first.getContig(); | ||
| int start = first.getStart(); | ||
| int end = start; | ||
| int end = first.getEnd(); |
There was a problem hiding this comment.
I definitely didn't notice it in review. It makes sense, you can't have an end before start..
Codecov Report
@@ Coverage Diff @@
## master #1318 +/- ##
===========================================
Coverage 67.635% 67.635%
Complexity 8230 8230
===========================================
Files 562 562
Lines 33617 33617
Branches 5642 5642
===========================================
Hits 22737 22737
Misses 8706 8706
Partials 2174 2174
|
8b83dd9 to
198a2e1
Compare
|
I added some more test cases to cover the cases you mentioned |
|
@yfarjoun I added the tests I think you wanted. Is there anything else? |
| {Arrays.asList(zeroLengthInterval, interval600To601), true, interval600To625}, | ||
| {Arrays.asList(interval600To601, new Interval(contig, 100, 200, false, "hasName")), true, new Interval(contig, 100, 601, false, "hasName")}, | ||
| {Arrays.asList(interval600To601, new Interval(contig, 100, 200, false, "hasName")), false, new Interval(contig, 100, 601, false, "hasName")}, | ||
| {Arrays.asList(zeroInterval10To9, new Interval(contig, 11, 15)), false, new Interval(contig, 10, 15)}, |
There was a problem hiding this comment.
I don't think that this is right. adding a zero-length interval shouldn't add bases to another interval.
* Fixing a bug in IntervalList.merge() that caused it to break when merging 0 length intervals * Improving tests for merge * The bug was introduced in #1265
198a2e1 to
8cca313
Compare
Responded to comments as well as I can here, punting the rest to a new pr
|
👍 |
3 participants
Checklist