Add a new interval list scatter mode #1786

ldgauthier · 2022-03-15T18:21:54Z

Add a new interval list scatter mode to avoid issue of giant final list in large joint genotyping scatters.
The WARP WGS joint genotyping workflow has a large (~20K) list of intervals of approximately even runtime, which we split by interval count for N < 20K scatters. However, because the INTERVAL_COUNT scatter mode does not allow the number of output lists to exceed the requested scatter count, the final list with the remainder can be very large with a prohibitively long runtime. For example, scattering 5980 ways for 2300 samples yields a final list that contains chromosomes 19, 20, 21, 22, X AND Y. Here we allow more interval lists to be returned so that they are all contain the same number of intervals for shards with reasonable runtimes.

Checklist (never delete this)

Never delete this, it is our record that procedure was followed. If you find that for whatever reason one of the checklist points doesn't apply to your PR, you can leave it unchecked but please add an explanation below.

Content

Added or modified tests to cover changes and any new functionality
Edited the README / documentation (if applicable)
All tests passing on Travis

Review

Final thumbs-up from reviewer
Rebase, squash and reword as applicable

For more detailed guidelines, see https://github.com/broadinstitute/picard/wiki/Guidelines-for-pull-requests

in large joint genotyping scatters

requested number of intervals

kachulis

This looks good @ldgauthier, just some minor requests.

kachulis · 2022-03-16T19:19:07Z

src/main/java/picard/util/IntervalList/IntervalListScatterMode.java

+
+    /***
+     * A scatter by interval **count** which attempts to fill each resulting interval list with approximately equal numbers
+     * of intervals, disregarding the base count. This approach distributes the remainder lists across the final interval lists.


Suggested change

* of intervals, disregarding the base count. This approach distributes the remainder lists across the final interval lists.

* of intervals, disregarding the base count. This approach distributes the remainder intervals across the initial interval lists.

kachulis · 2022-03-16T19:19:58Z

src/main/java/picard/util/IntervalList/IntervalListScatterMode.java

+     */
+    INTERVAL_COUNT_WITH_DISTRIBUTED_REMAINDER(IntervalListScattererByIntervalCountWithDistributedRemainder::new, "Scatter the interval list into similarly sized interval lists " +
+            "(by interval count, not by base count). " +
+            "Resulting interval lists will contain similar number of intervals but may exceed the requested number of scatters to account for the remainder.");


should no longer exceed the requested number of scatters

kachulis · 2022-03-16T19:20:55Z

src/main/java/picard/util/IntervalListTools.java

@@ -467,7 +467,7 @@ protected int doWork() {
            LOG.info(String.format("Wrote %s scatter subdirectories to %s.", scattered.size, OUTPUT));
            if (scattered.size != SCATTER_COUNT) {
                LOG.warn(String.format(
-                        "Requested scatter width of %s, but only emitted %s.  (This may be an expected consequence of running in %s mode.)",


do you still want to remove "only"?

True, now that the new mode doesn't output extra, that "only" is still appropriate. Good catch.

kachulis · 2022-03-16T19:22:29Z

src/test/java/picard/util/IntervalListScattererTest.java

@@ -393,6 +403,8 @@ private Testcase(final File file, final int scatterWidth, final IntervalListScat
                                IntervalList.overlaps(LIST_TO_SCATTER_MANY, secondThird))
                        ))))));

+
+


extra newlines

kachulis · 2022-03-16T19:24:51Z

testdata/picard/util/test.hg38.200.intervals

@@ -0,0 +1,200 @@
+chr1:1-195878


is this file used of anything?

kachulis · 2022-03-16T19:25:56Z

testdata/picard/util/largeScatters/temp_0001_of_60/scattered.interval_list

@@ -0,0 +1,6 @@
+@HD	VN:1.6	SO:coordinate
+@SQ	SN:chr1	LN:248956422	M5:6aef897c3d6ff0c78aff06ac189178dd	AS:38	UR:/seq/references/Homo_sapiens_assembly38/v0/Homo_sapiens_assembly38.fasta	SP:Homo sapiens


sanitize headers if it's not too difficult

Admittedly the /seq/references path isn't super helpful for most picard contributors, but I like having the info that's there for reproducibility.

kachulis · 2022-03-17T19:29:26Z

...a/picard/util/IntervalList/IntervalListScattererByIntervalCountWithDistributedRemainder.java

+
+    @Override
+    public List<Interval> takeSome(final Interval interval, final long idealSplitWeight, final long currentSize, final double projectSizeOfRemaining) {
+        final double amount = projectSizeOfRemaining - currentSize;


I know you're just copying this structure from IntervalListScattererByIntervalCount, but this feels like a very roundabout way to write if (projectSizeOfRemaining > currentSize)

kachulis · 2022-03-17T19:32:50Z

src/test/java/picard/util/IntervalListScattererTest.java

+                LARGER_INTERVAL_FILE, 60, IntervalListScatterMode.INTERVAL_COUNT_WITH_DISTRIBUTED_REMAINDER,
+                LARGER_EXPECTED_LISTS
+        ));
+


add a test where there is no remainder, and make sure all interval_lists end up the same size

In a new test, I split the 200 intervals into 20 lists and verified that they each have 10 intervals.

w.r.t. scatter count versus scatter content

kachulis

Looks good @ldgauthier! One small comment about a behavior change, but I think it is making the behavior more correct, so I'm ok with it. And I opened #1795 to document the test issues we've been discussing, and hopefully get them fixed someday.

kachulis · 2022-04-07T14:22:13Z

src/main/java/picard/util/IntervalListTools.java

@@ -455,7 +455,7 @@ protected int doWork() {

        if (SCATTER_CONTENT != null) {
            final long listSize = SUBDIVISION_MODE.make().listWeight(output);
-            SCATTER_COUNT = (int) listSize / SCATTER_CONTENT;
+            SCATTER_COUNT = (int)Math.round((double) listSize / SCATTER_CONTENT);


This will change behavior. I agree that it is more correct this way, but just want to flag it.

Add a new interval list scatter mode to avoid issue of giant final list

d6782e1

in large joint genotyping scatters

ldgauthier requested a review from kachulis March 15, 2022 18:39

ldgauthier added 2 commits March 15, 2022 14:40

New repo, same mistake

b890b3a

More elegant approach (suggested by Chris) to output exactly the

e606d22

requested number of intervals

gbggrant assigned kachulis Mar 16, 2022

ldgauthier added 2 commits March 17, 2022 12:39

Missed a constant change

42510ca

More elegant -- bigger lists up front

65b1eed

kachulis requested changes Mar 17, 2022

View reviewed changes

ldgauthier added 2 commits March 18, 2022 10:56

Address review comments

e79d166

Refactor to allow for non-symmetric behavior of new remainder mode

45e335e

w.r.t. scatter count versus scatter content

ldgauthier mentioned this pull request Mar 30, 2022

Fix all the (scatter) things! broadinstitute/warp#299

Merged

ldgauthier mentioned this pull request Apr 6, 2022

Upgrade google-cloud-nio to fix broken requester pays support #1792

Merged

5 tasks

kachulis mentioned this pull request Apr 7, 2022

IntervalListToolsTest.testScatterByContent and IntervalListToolsTest.testScatter could use some improvement #1795

Open

kachulis approved these changes Apr 7, 2022

View reviewed changes

Merge branch 'master' into ldg_newScatterMode

97a508b

ldgauthier merged commit 9eafe4e into master Apr 7, 2022

ldgauthier deleted the ldg_newScatterMode branch April 7, 2022 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new interval list scatter mode #1786

Add a new interval list scatter mode #1786

ldgauthier commented Mar 15, 2022

kachulis left a comment

kachulis Mar 16, 2022

kachulis Mar 16, 2022

kachulis Mar 16, 2022

ldgauthier Mar 18, 2022

kachulis Mar 16, 2022

kachulis Mar 16, 2022

ldgauthier Mar 18, 2022 •

edited

Loading

kachulis Mar 16, 2022

ldgauthier Mar 18, 2022

kachulis Mar 17, 2022

kachulis Mar 17, 2022

ldgauthier Mar 18, 2022

kachulis left a comment

kachulis Apr 7, 2022

	* of intervals, disregarding the base count. This approach distributes the remainder lists across the final interval lists.
	* of intervals, disregarding the base count. This approach distributes the remainder intervals across the initial interval lists.

		@@ -393,6 +403,8 @@ private Testcase(final File file, final int scatterWidth, final IntervalListScat
		IntervalList.overlaps(LIST_TO_SCATTER_MANY, secondThird))
		))))));

		@@ -0,0 +1,6 @@
		@HD VN:1.6 SO:coordinate
		@SQ SN:chr1 LN:248956422 M5:6aef897c3d6ff0c78aff06ac189178dd AS:38 UR:/seq/references/Homo_sapiens_assembly38/v0/Homo_sapiens_assembly38.fasta SP:Homo sapiens

Add a new interval list scatter mode #1786

Add a new interval list scatter mode #1786

Conversation

ldgauthier commented Mar 15, 2022

Checklist (never delete this)

Content

Review

kachulis 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

ldgauthier Mar 18, 2022 • edited Loading

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

kachulis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldgauthier Mar 18, 2022 •

edited

Loading