Added IntervalMergingRule.NONE as an advanced use case #5887

jamesemery · 2019-04-16T21:41:11Z

Added the ability to specify IntervalMergingRule.NONE so so that no merging is performed. Also added the ability to request from IntervalArgumentCollection the unmerged user intervals. I have not solved the underlying issue that the GenomeLocSet requires non-overlapping intervals, though I acknowledge that replacing or refactoring that class is the long term solution to this problem.

codecov-io · 2019-04-16T23:48:46Z

Codecov Report

❗ No coverage uploaded for pull request base (master@dcff818). Click here to learn what that means.
The diff coverage is 92.708%.

@@            Coverage Diff             @@
##             master     #5887   +/-   ##
==========================================
  Coverage          ?   86.825%           
  Complexity        ?     32305           
==========================================
  Files             ?      1991           
  Lines             ?    149187           
  Branches          ?     16484           
==========================================
  Hits              ?    129531           
  Misses            ?     13641           
  Partials          ?      6015

Impacted Files	Coverage Δ	Complexity Δ
...titute/hellbender/utils/IntervalUtilsUnitTest.java	`91.906% <100%> (ø)`	`146 <0> (?)`
...nstitute/hellbender/utils/IntervalMergingRule.java	`100% <100%> (ø)`	`1 <0> (?)`
...broadinstitute/hellbender/utils/IntervalUtils.java	`92.083% <100%> (ø)`	`192 <4> (?)`
...rgumentcollections/IntervalArgumentCollection.java	`89.063% <88.636%> (ø)`	`24 <1> (?)`
...entcollections/IntervalArgumentCollectionTest.java	`90.411% <94.118%> (ø)`	`26 <5> (?)`

droazen

Back to @jamesemery with my comments

...va/org/broadinstitute/hellbender/cmdline/argumentcollections/IntervalArgumentCollection.java

...rg/broadinstitute/hellbender/cmdline/argumentcollections/IntervalArgumentCollectionTest.java

droazen · 2019-12-19T19:33:00Z

src/test/java/org/broadinstitute/hellbender/utils/IntervalUtilsUnitTest.java

@@ -1115,6 +1115,9 @@ public void testUnmergedIntervals(String unmergedIntervals) {
        merged = IntervalUtils.mergeIntervalLocations(locs, IntervalMergingRule.ALL);
        Assert.assertEquals(merged.size(), 1);

+        merged = IntervalUtils.mergeIntervalLocations(locs, IntervalMergingRule.NONE);
+        Assert.assertEquals(merged.size(), 2);
+


If you choose to keep the IntervalUtils.loadIntervalsNonMerging() method around, you'll need to add direct unit tests for it here.

...va/org/broadinstitute/hellbender/cmdline/argumentcollections/IntervalArgumentCollection.java

droazen · 2019-12-19T19:41:43Z

@lbergelson In your opinion, how likely is this feature to cause problems? We do still call QueryInterval.optimizeIntervals() to merge intervals in ReadsDataSource before starting an iteration, and I think that's the main example of an HTSJDK query interface that can't handle overlapping intervals.

droazen · 2019-12-19T20:14:21Z

@jamesemery As discussed in person, I think we should remove NONE as a merging rule, and just keep the IntervalArgumentCollection.getSpecifiedIntervalsWithoutMerging() method around for tools that need it.

lbergelson · 2019-12-19T20:26:38Z

I think this will not be exactly what people expect. I think if I saw this, I would expect that I would get apply calls for each position that was in the non-merged intervals. I.e. I would get multiple applies at the same position if I had overlapping intervals. I would like to have a MergingRule that does that, but if we don't take care to make the read queries individual to each interval then this will be confusing.

jamesemery · 2019-12-19T20:47:31Z

Alright, I think I am in agreement with you @lbergelson about this behavior. Furthermore, all I needed out of this branch was an exposed mechanism for getting back the un-merged intervals so that I can track them myself in subtools. To that end I think I'm going to keep this branch and its tests and get rid of the merging rule argument in favor of leaving the logic for merging in place to be accessed by tools.

jamesemery · 2019-12-19T21:26:56Z

@droazen back to you

droazen · 2019-12-19T21:33:44Z

...va/org/broadinstitute/hellbender/cmdline/argumentcollections/IntervalArgumentCollection.java

+     */
+    public List<SimpleInterval> getIntervalsWithoutMerging(final SAMSequenceDictionary sequenceDict ) {
+        if (!intervalsSpecified()) {
+            throw new GATKException("Cannot call getIntervalsWithoutMerging() without specifying either intervals to include or exclude.");


This should check whether getIntervalStrings() is non-empty to make sure that there are intervals to include. The error message needs to be adjusted as well:

"without specifying either intervals to include or exclude" -> " without specifying intervals to include"

Should the method also throw if there are exclusion intervals specified, or should it just silently ignore them? I leave that up to you...

droazen

@jamesemery Found some lingering issues in the test code. We can merge this after they're addressed.

...roadinstitute/hellbender/cmdline/argumentcollections/IntervalArgumentCollectionUnitTest.java

droazen · 2019-12-19T21:48:41Z

src/test/java/org/broadinstitute/hellbender/utils/IntervalUtilsUnitTest.java

+                new Object[]{Arrays.asList("1:1-2", "2:1-2"), 0, intervalStringsToGenomeLocs("1:1-2", "2:1-2")},
+                new Object[]{Arrays.asList("1:1-10", "1:5-15"), 0, intervalStringsToGenomeLocs("1:1-10", "1:5-15")},
+                new Object[]{Arrays.asList("1:5-5"), 5, intervalStringsToGenomeLocs("1:1-10")},
+                new Object[]{Arrays.asList(severalIntervals), 0, intervalStringsToGenomeLocs("1:100-200", "2:20-30", "4:50")}


Only one of these test cases has an overlapping interval, which seems inadequate given the nature of the method being tested.

This method is adequately tested in the other class (which is just directly calling into this method which does all of the actual work). I will add a few more redundant cases here but I am confident that those tests are checking what I actually want.

gatk-bot · 2019-12-19T22:06:36Z

Travis reported job failures from build 28430
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	28430.13	logs
unit	openjdk8	28430.3	logs

gatk-bot · 2019-12-19T22:20:09Z

Travis reported job failures from build 28433
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	28433.13	logs
unit	openjdk8	28433.3	logs

gatk-bot · 2019-12-19T22:26:58Z

Travis reported job failures from build 28432
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
unit	openjdk11	28432.13	logs
unit	openjdk8	28432.3	logs

…d back down again

jamesemery · 2019-12-20T15:47:15Z

@droazen I resolved your comments and fixed the test I broke. Can this be merged so we can move forward on that other branch?

droazen

One final comment/question @jamesemery, then this is good to go.

droazen · 2019-12-20T16:15:08Z

...va/org/broadinstitute/hellbender/cmdline/argumentcollections/IntervalArgumentCollection.java

+     */
+    public List<SimpleInterval> getIntervalsWithoutMerging(final SAMSequenceDictionary sequenceDict ) {
+        if (getIntervalStrings().isEmpty() ) {
+            throw new GATKException("Cannot call getIntervalsWithoutMerging() without specifying intervals to include.");


The regular getIntervals() method uses the reference dictionary to construct a set of intervals when getIntervalStrings() is empty, rather than throwing an exception. Should this method do the same for the sake of consistency? (ie., return GenomeLocSortedSet.createSetFromSequenceDictionary(sequenceDict).toList() in that case?)

It could potentially, for the purposes of Depth of Coverage I do not need this functionality as it requires intervals be specified. If i need to change it perhaps I will switch it there? I would really like to get this in so Depth of Coverage can be looked at.

jamesemery requested a review from droazen April 16, 2019 21:41

droazen self-assigned this Apr 17, 2019

samuelklee mentioned this pull request Apr 18, 2019

Update CNV tools and documentation to use IntervalMergingRule.NONE. #5891

Closed

jamesemery mentioned this pull request May 2, 2019

DepthOfCoverage port #5913

Merged

droazen suggested changes Dec 19, 2019

View reviewed changes

droazen assigned jamesemery and unassigned droazen Dec 19, 2019

jamesemery added 2 commits December 19, 2019 16:25

added a kludgy option for not doing any interval merging if you want

008664e

responded to comments

a88b534

jamesemery assigned droazen and unassigned jamesemery Dec 19, 2019

jamesemery force-pushed the je_allowUnmergedIntervals branch from d3c9ee4 to a88b534 Compare December 19, 2019 21:27

droazen reviewed Dec 19, 2019

View reviewed changes

jamesemery added 2 commits December 19, 2019 16:39

responded to comment round 2

5409df5

fixed the errant either

c9e2560

droazen suggested changes Dec 19, 2019

View reviewed changes

droazen assigned jamesemery and unassigned droazen Dec 19, 2019

pushing the boulder up the hill because by morning it will have rolle…

c617856

…d back down again

jamesemery removed their assignment Dec 20, 2019

droazen approved these changes Dec 20, 2019

View reviewed changes

droazen assigned jamesemery Dec 20, 2019

jamesemery merged commit a000a87 into master Dec 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added IntervalMergingRule.NONE as an advanced use case #5887

Added IntervalMergingRule.NONE as an advanced use case #5887

jamesemery commented Apr 16, 2019

codecov-io commented Apr 16, 2019 •

edited by codecov bot

Loading

droazen left a comment

droazen Dec 19, 2019

droazen commented Dec 19, 2019

droazen commented Dec 19, 2019

lbergelson commented Dec 19, 2019

jamesemery commented Dec 19, 2019

jamesemery commented Dec 19, 2019

droazen Dec 19, 2019

droazen left a comment

droazen Dec 19, 2019

jamesemery Dec 20, 2019

gatk-bot commented Dec 19, 2019 •

edited

Loading

gatk-bot commented Dec 19, 2019 •

edited

Loading

gatk-bot commented Dec 19, 2019 •

edited

Loading

jamesemery commented Dec 20, 2019

droazen left a comment

droazen Dec 20, 2019

jamesemery Dec 20, 2019

Added IntervalMergingRule.NONE as an advanced use case #5887

Added IntervalMergingRule.NONE as an advanced use case #5887

Conversation

jamesemery commented Apr 16, 2019

codecov-io commented Apr 16, 2019 • edited by codecov bot Loading

Codecov Report

droazen left a comment

Choose a reason for hiding this comment

droazen Dec 19, 2019

Choose a reason for hiding this comment

droazen commented Dec 19, 2019

droazen commented Dec 19, 2019

lbergelson commented Dec 19, 2019

jamesemery commented Dec 19, 2019

jamesemery commented Dec 19, 2019

droazen Dec 19, 2019

Choose a reason for hiding this comment

droazen left a comment

Choose a reason for hiding this comment

droazen Dec 19, 2019

Choose a reason for hiding this comment

jamesemery Dec 20, 2019

Choose a reason for hiding this comment

gatk-bot commented Dec 19, 2019 • edited Loading

gatk-bot commented Dec 19, 2019 • edited Loading

gatk-bot commented Dec 19, 2019 • edited Loading

jamesemery commented Dec 20, 2019

droazen left a comment

Choose a reason for hiding this comment

droazen Dec 20, 2019

Choose a reason for hiding this comment

jamesemery Dec 20, 2019

Choose a reason for hiding this comment

codecov-io commented Apr 16, 2019 •

edited by codecov bot

Loading

gatk-bot commented Dec 19, 2019 •

edited

Loading

gatk-bot commented Dec 19, 2019 •

edited

Loading

gatk-bot commented Dec 19, 2019 •

edited

Loading