From fe27e66378739c353a78f54e38c5a3b2cb619956 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Mon, 11 Mar 2019 11:44:08 -0400 Subject: [PATCH] Fixing zero-length interval bug in IntervalList.merge (#1318) * Fixing zero-length interval bug in IntervalList.merge * 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 * improving comment in IntervalList --- .../htsjdk/samtools/util/IntervalList.java | 9 ++- .../samtools/util/IntervalListTest.java | 55 ++++++++++++++----- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/src/main/java/htsjdk/samtools/util/IntervalList.java b/src/main/java/htsjdk/samtools/util/IntervalList.java index 88a0626695..8b555b5142 100644 --- a/src/main/java/htsjdk/samtools/util/IntervalList.java +++ b/src/main/java/htsjdk/samtools/util/IntervalList.java @@ -379,13 +379,18 @@ private static List breakIntervalAtBandMultiples(final Interval interv } /** - * Merges a sorted collection of intervals and optionally concatenates unique names or takes the first name. + * Merges a collection of intervals and optionally concatenates unique names or takes the first name. + * * + * @param concatenateNames if true, combine the names of all the intervals with |, otherwise use the name of the first interval. + * @return a single interval which spans from the minimum input start position to the maximum input end position. + * The resulting strandedness and contig are those of the first input with no validation. + * */ static Interval merge(final Iterable intervals, final boolean concatenateNames) { final Interval first = intervals.iterator().next(); final String chrom = first.getContig(); int start = first.getStart(); - int end = start; + int end = first.getEnd(); final boolean neg = first.isNegativeStrand(); final LinkedHashSet names = new LinkedHashSet<>(); final String name; diff --git a/src/test/java/htsjdk/samtools/util/IntervalListTest.java b/src/test/java/htsjdk/samtools/util/IntervalListTest.java index 3ff230a00e..2632076d04 100644 --- a/src/test/java/htsjdk/samtools/util/IntervalListTest.java +++ b/src/test/java/htsjdk/samtools/util/IntervalListTest.java @@ -564,22 +564,47 @@ public void testFromSequenceName(final Path intervalList, final String reference Assert.assertEquals(test.getIntervals(), CollectionUtil.makeList(new Interval(referenceName, 1, length))); } - @Test - public void testMerges() { - final SortedSet intervals = new TreeSet() {{ - add(new Interval("1", 500, 600, false, "foo")); - add(new Interval("1", 550, 650, false, "bar")); - add(new Interval("1", 625, 699, false, "splat")); - }}; - - Interval out = IntervalList.merge(intervals, false); - Assert.assertEquals(out.getStart(), 500); - Assert.assertEquals(out.getEnd(), 699); + @DataProvider + public Object[][] getMergeTestCases() { + final String contig = "1"; + final Interval foo = new Interval(contig, 500, 600, false, "foo"); + final Interval bar = new Interval(contig, 550, 650, false, "bar"); + final Interval splat = new Interval(contig, 625, 699, false, "splat"); + final List threeInOrderIntervals = Arrays.asList(foo, bar, splat); + final List threeOutOfOrderIntervals = Arrays.asList(bar, foo, splat); + final Interval zeroLengthInterval = new Interval(contig, 626, 625); + final Interval interval600To601 = new Interval(contig, 600, 601); + final Interval interval600To625 = new Interval(contig, 600, 625); + final Interval normalInterval = new Interval(contig, 626, 629, true, "whee"); + final Interval zeroInterval10To9 = new Interval(contig, 10, 9); + return new Object[][]{ + {threeInOrderIntervals, true, new Interval(contig, 500, 699, false, "foo|bar|splat")}, + {threeInOrderIntervals, false, new Interval(contig, 500, 699, false, "foo")}, + {threeOutOfOrderIntervals, true, new Interval(contig, 500, 699, false, "bar|foo|splat")}, + {threeOutOfOrderIntervals, false, new Interval(contig, 500, 699, false, "bar")}, + {Collections.singletonList(normalInterval), true, normalInterval}, + {Collections.singletonList(normalInterval), false, normalInterval}, + {Collections.singletonList(zeroLengthInterval), true, zeroLengthInterval}, + {Collections.singletonList(zeroLengthInterval), false, zeroLengthInterval}, + {Arrays.asList(zeroLengthInterval, interval600To601), true, interval600To625}, + {Arrays.asList(zeroLengthInterval, interval600To601), false, interval600To625}, + {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)}, + {Arrays.asList(zeroInterval10To9, new Interval(contig, 10, 15)), true, new Interval(contig, 10, 15)}, + {Arrays.asList(zeroInterval10To9, new Interval(contig, 9,15)), false, new Interval(contig, 9, 15)}, + {Arrays.asList(zeroInterval10To9, new Interval(contig, 8, 9)), true, new Interval(contig, 8, 9)} + }; + } - intervals.add(new Interval("1", 626, 629, false, "whee")); - out = IntervalList.merge(intervals, false); - Assert.assertEquals(out.getStart(), 500); - Assert.assertEquals(out.getEnd(), 699); + @Test(dataProvider = "getMergeTestCases") + public void testMerges(Iterable intervals, boolean concatNames, Interval expected) { + final Interval merged = IntervalList.merge(intervals, concatNames); + Assert.assertEquals(merged.getContig(), expected.getContig()); + Assert.assertEquals(merged.getStart(), expected.getStart()); + Assert.assertEquals(merged.getStrand(), expected.getStrand()); + Assert.assertEquals(merged.getName(), expected.getName()); } @Test