FilterIntervals now filters out any singleton intervals #6559

asmirnov239 · 2020-04-21T16:40:09Z

This implements suggestion #2 proposed by @samuelklee from issue #5852

asmirnov239 · 2020-04-21T16:41:59Z

@samuelklee @mwalker174 Could you please review? Also does either of you know if ScatterIntervals task in CNVTasks WDL has a potential of producing singleton intervals?

gatk-bot · 2020-04-21T17:15:41Z

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

Test Type	JDK	Job ID	Logs
python	openjdk8	30090.5	logs

samuelklee · 2020-04-21T17:39:28Z

@asmirnov239 looks like you are getting an NPE---remember that intervals are resolved after argument validation, so you need to do the check later.

Also, good point, I think you can get a singleton after scattering if you get unlucky with your shards.
Perhaps change the check to a filtering step in GermlineCNVCaller?

asmirnov239 · 2020-04-21T17:56:23Z

@samuelklee Thank you, that's fixed now.

Regarding ScatterIntervals issue - what do you think about changing IntervalListTools logic to not produce singletons instead of filtering them out in GermlineCNVCaller?

samuelklee · 2020-04-21T18:00:26Z

@asmirnov239 I think that we probably want all shard sizes to be as close to equal as possible, so that we avoid any possible issues arising from different model capacities across shards.

Actually, is this even an issue at the GermlineCNVCaller step? Maybe we just need to worry about it during ploidy/postprocessing?

… other intervals on their contigs

asmirnov239 · 2020-04-22T22:48:02Z

@samuelklee You're right - I tested it and it's only an issue in postprocessing , so I'm only making a check there. Neither GermlineCNVCaller or DetermineContigPloidy code fails due to this issue.

It should be all set to be reviewed now.

samuelklee

Just some minor nitpicky stuff. Otherwise, thanks, looks good to me!

samuelklee · 2020-04-30T18:59:07Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervals.java

@@ -431,6 +432,17 @@ private SimpleIntervalCollection filterIntervals() {
                    countNumberPassing(mask), numIntersectedIntervals));
        }

+        //finally, filter intervals that are solitary in their corresponding contigs
+        final Map<String, Long> contigToCountMap = IntStream.range(0, numIntersectedIntervals)
+                .filter(i -> !mask[i]).mapToObj(i -> intersectedIntervals.getRecords().get(i))


Put mapToObj on its own line for consistency.

samuelklee · 2020-04-30T18:59:27Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervals.java

+        final Map<String, Long> contigToCountMap = IntStream.range(0, numIntersectedIntervals)
+                .filter(i -> !mask[i]).mapToObj(i -> intersectedIntervals.getRecords().get(i))
+                .collect(Collectors.groupingBy(SimpleInterval::getContig, Collectors.counting()));
+        IntStream.range(0, numIntersectedIntervals).filter(i -> !mask[i]).forEach(i -> {


Break up filter and forEach onto their own lines.

samuelklee · 2020-04-30T19:01:39Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervals.java

+                .filter(i -> !mask[i]).mapToObj(i -> intersectedIntervals.getRecords().get(i))
+                .collect(Collectors.groupingBy(SimpleInterval::getContig, Collectors.counting()));
+        IntStream.range(0, numIntersectedIntervals).filter(i -> !mask[i]).forEach(i -> {
+            final long value = contigToCountMap.get(intersectedIntervals.getRecords().get(i).getContig());


I might just name this variable count. Perhaps even intervalCount, and rename the map to contigToIntervalCountMap.

Done and done

samuelklee · 2020-04-30T19:03:25Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervals.java

+                .collect(Collectors.groupingBy(SimpleInterval::getContig, Collectors.counting()));
+        IntStream.range(0, numIntersectedIntervals).filter(i -> !mask[i]).forEach(i -> {
+            final long value = contigToCountMap.get(intersectedIntervals.getRecords().get(i).getContig());
+            if (value <= 1) {


Seems a bit weird to have <= since it should never happen (and I can't imagine future code changes that would make it possible), but no biggie.

Changed it to ==

samuelklee · 2020-04-30T19:04:24Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervals.java

+        IntStream.range(0, numIntersectedIntervals).filter(i -> !mask[i]).forEach(i -> {
+            final long value = contigToCountMap.get(intersectedIntervals.getRecords().get(i).getContig());
+            if (value <= 1) {
+                mask[i] = true;


Perhaps a log message here indicating when a contig is dropped?

samuelklee · 2020-04-30T19:09:02Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java

+            concatenatedIntervalList.addAll(intervalCollections.get(i).getIntervals());
+        });
+
+        final Map<String, Long> contigToCountMap = concatenatedIntervalList.stream()


I'd make the variable names and code here consistent with the code in FilterIntervals.

Okay I did that as much as possible

samuelklee · 2020-04-30T19:09:57Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java

@@ -654,4 +653,25 @@ private static File getCopyNumberSegmentsFile(final File pythonSegmenterOutputPa
        }
        return unsortedIntervalCollectionsFromModels;
    }
+
+    /**
+     * Validate that the union of shard's interval lists does not have singleton intervals, i.e. intervals that


Validate that the concatenation of the sharded interval lists does not have singleton intervals..., perhaps?

samuelklee · 2020-04-30T19:10:19Z

...test/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervalsIntegrationTest.java

@@ -27,6 +27,7 @@
 import org.testng.annotations.Test;

 import java.io.File;
+import java.lang.reflect.AnnotatedElement;


Stray import?

Hmm not sure how that got in there. Thanks!

samuelklee · 2020-04-30T19:10:37Z

...test/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervalsIntegrationTest.java

@@ -71,6 +72,16 @@
                            new AnnotationMap(Arrays.asList(
                                    Pair.of(CopyNumberAnnotations.GC_CONTENT, 0.95),
                                    Pair.of(CopyNumberAnnotations.MAPPABILITY, 0.95),
+                                    Pair.of(CopyNumberAnnotations.SEGMENTAL_DUPLICATION_CONTENT, 0.5)))),
+                    new AnnotatedInterval(new SimpleInterval("20", 51,	60),


White space. Thanks for adding these tests!

Oh the other ones had tabs as well.. fixed!

samuelklee · 2020-04-30T19:12:04Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervals.java

- * other tools (e.g., {@link DetermineGermlineContigPloidy} and {@link GermlineCNVCaller}) to mask intervals from
- * analysis.
+ * Annotation-based filters will be applied first, followed by count-based filters. In the end, any singleton intervals
+ * (i.e. those being by themselves on their corresponding contigs) found after applying other filters will be filtered


Comma after i.e..

mwalker174

Thank you @asmirnov239, I have a few questions/comments.

mwalker174 · 2020-04-30T19:16:24Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervals.java

+        IntStream.range(0, numIntersectedIntervals).filter(i -> !mask[i]).forEach(i -> {
+            final long value = contigToCountMap.get(intersectedIntervals.getRecords().get(i).getContig());
+            if (value <= 1) {
+                mask[i] = true;


Log a warning here, something like:

Filtered singleton interval on contig: "chrX"

mwalker174 · 2020-04-30T19:17:09Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java

@@ -246,7 +243,7 @@
    public void onStartup() {
        super.onStartup();
        /* check for successful import of gcnvkernel */
-        PythonScriptExecutor.checkPythonEnvironmentForPackage("gcnvkernel");
+        PythonScriptExecutor.checkPythonEnvironmentForPackage("numpy");


Why this change?

Oops, this was to get it to run in IDE. Thanks for catching this!

mwalker174 · 2020-04-30T19:17:59Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java

+     * Validate that the union of shard's interval lists does not have singleton intervals, i.e. intervals that
+     * are the only ones on their corresponding contigs.
+     */
+    private void checkForSingletonIntervalAbsence(final List<SimpleIntervalCollection> intervalCollections){


I'd argue you can just call this checkForSingletonInterval

mwalker174 · 2020-04-30T19:23:07Z

src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java

+        final List<SimpleInterval> concatenatedIntervalList = new ArrayList<>();
+        IntStream.range(0, numShards).forEach(i -> {
+            concatenatedIntervalList.addAll(intervalCollections.get(i).getIntervals());
+        });
+
+        final Map<String, Long> contigToCountMap = concatenatedIntervalList.stream()
+                .collect(Collectors.groupingBy(SimpleInterval::getContig, Collectors.counting()));
+        contigToCountMap.keySet().forEach(c -> {
+            if (contigToCountMap.get(c) == 1) {
+                throw new IllegalArgumentException(
+                        String.format("Records contain a singleton interval on contig (%s)." +
+                                " Please run FilterIntervals tool first.", c));
+            }
+        });


Can be simplified a bit:

intervalCollections.stream() .flatMap(list -> list.getIntervals().stream()) .collect(Collectors.groupingBy(SimpleInterval::getContig, Collectors.counting())) .entrySet().stream() .forEach(entry -> { if (entry.getValue() == 1) { throw new IllegalArgumentException( String.format("Records contain a singleton interval on contig (%s)." + " Please run FilterIntervals tool first.", entry.getKey())); } });

Nice, I used this

mwalker174 · 2020-04-30T19:26:50Z

...test/java/org/broadinstitute/hellbender/tools/copynumber/FilterIntervalsIntegrationTest.java

+                {intervalsWithExtraIntervalFile, Collections.emptyList(), annotatedIntervalsFile, 0.1, 0.9, 0.1, 0.9, 0.1, 0.9, Arrays.asList(2, 5)},
+                {intervalsWithExtraIntervalFile, Collections.singletonList("20:1-10"), annotatedIntervalsFile, 0., 1., 0., 1., 0., 1., Arrays.asList(1, 2, 3, 4, 5)},
+                {intervalsWithExtraIntervalFile, Arrays.asList("20:1-15", "20:35-45"), annotatedIntervalsFile, 0., 1., 0., 1., 0., 1., Arrays.asList(2, 5)},
+                {intervalsWithExtraIntervalFile, Collections.singletonList("20:25-50"), annotatedIntervalsFile, 0.1, 0.9, 0., 1., 0., 1., Arrays.asList(0, 1, 5)}};


Does one of these test for the singleton case?

All of them actually do because I added an interval from contig "21" which should be filtered out in all of these.

gatk-bot · 2020-06-02T22:34:38Z

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

Test Type	JDK	Job ID	Logs
integration	openjdk11	30489.11	logs
integration	openjdk8	30489.2	logs

* FilterIntervals now filters out any singleton intervals, that have no other intervals on their contigs.

asmirnov239 requested review from mwalker174 and samuelklee April 21, 2020 16:40

FilterIntervals now filters out any singleton intervals, that have no…

8ac73b5

… other intervals on their contigs

asmirnov239 force-pushed the as_filter_single_interval_contigs branch from 151fed2 to 8ac73b5 Compare April 22, 2020 21:45

samuelklee approved these changes Apr 30, 2020

View reviewed changes

mwalker174 reviewed Apr 30, 2020

View reviewed changes

Addressed PR comments

028f9f9

droazen assigned asmirnov239, samuelklee and mwalker174 and unassigned samuelklee Jun 3, 2020

asmirnov239 merged commit 189ea45 into master Jun 12, 2020

asmirnov239 deleted the as_filter_single_interval_contigs branch June 12, 2020 19:42

asmirnov239 mentioned this pull request Jun 12, 2020

Throw exception when contig consists of only a single interval in gCNV tools. #5852

Closed

jonn-smith pushed a commit that referenced this pull request Jul 14, 2020

FilterIntervals now filters out any singleton intervals (#6559)

2fcdf89

* FilterIntervals now filters out any singleton intervals, that have no other intervals on their contigs.

FilterIntervals now filters out any singleton intervals #6559

FilterIntervals now filters out any singleton intervals #6559

Conversation

asmirnov239 commented Apr 21, 2020 • edited Loading

asmirnov239 commented Apr 21, 2020

gatk-bot commented Apr 21, 2020

samuelklee commented Apr 21, 2020 • edited Loading

asmirnov239 commented Apr 21, 2020 • edited Loading

samuelklee commented Apr 21, 2020 • edited Loading

asmirnov239 commented Apr 22, 2020

samuelklee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwalker174 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatk-bot commented Jun 2, 2020 • edited Loading

asmirnov239 commented Apr 21, 2020 •

edited

Loading

samuelklee commented Apr 21, 2020 •

edited

Loading

asmirnov239 commented Apr 21, 2020 •

edited

Loading

samuelklee commented Apr 21, 2020 •

edited

Loading

gatk-bot commented Jun 2, 2020 •

edited

Loading