WeightedSplitInterval fixes [VS-384] [VS-332] #7795

mcovarr · 2022-04-19T23:34:54Z

EDIT: restored the experimental tool warning as we're not sure it's currently okay to remove that

Fixes a few issues found with WeightedSplitIntervals:

Fixes a crash processing the Charlie data set when it initially seems like more intervals could be added to the interval list under construction, but upon further examination it turns out there cannot
Fixes a glitch in writing interval name lines for split intervals when the parent interval name was null
~~Removes the experimental tool warning~~

mcovarr · 2022-04-19T23:35:47Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/WeightedInterval.java

@@ -61,15 +61,15 @@ public WeightedInterval[] split(int basesInFirstInterval) {
                this.getStart(),
                this.getStart() + basesInFirstInterval - 1,
                this.isNegativeStrand(),
-                this.getName() + "-1", // ensure names are unique
+                this.getName() == null ? null : this.getName() + "-1", // ensure non-null names are unique


interval list files were getting lines like

chr1 772635 877632 + null-2|null-1

which should have been

chr1 772635 877632 + .

nice catch---but are nulls okay here? Seems odd we were getting nulls to begin with?

WeightedSplitIntervals only uses the two-arg version of the WeightedInterval constructor which means the WeightedIntervals it creates always have a null name.

Would it make sense to give them names? Not necessary, but looks kind of odd without them.

I would defer to others on that, I am new enough to this domain that this didn't jump out as odd to me. 🙂

yeah I've always found the interval names in these files less than useful since unless your targeting biological constructs (ie gene names, etc) you typically end up doing something like "foo-XXX" which sort of is meaningless

mcovarr · 2022-04-19T23:36:50Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/WeightedSplitIntervals.java

+
+                if (basesToTake == 0) {
+                    // We can't add any more bases to the current interval list so put this interval back.
+                    iter.pushback(wi);


Fixes the Charlie crashing issue and the resulting interval list files are identical to those produced by the 86f1116 version of the gatk jar used for Beta.

looks great

mcovarr · 2022-04-19T23:37:02Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/WeightedSplitIntervals.java

@@ -41,7 +41,6 @@
        programGroup = IntervalsManipulationProgramGroup.class
 )
 @DocumentedFeature
-@ExperimentalFeature


experimental no more!

Talk to @lbergelson -- the Experimental tag in GATK means something specific about support. Many significant tools (reblocking I believe) are in this category because there is not full frontline support for them.

OK thanks... the team had expressed some concern about the seemingly alarming "experimental" warning message on startup in what now feels like a core part of a production(ish) pipeline.

I agree with that, and I think it's how this came up the last time :D It was something akin to Gmail being in Beta. But Louis or @droazen can give us the scoop

gatk-bot · 2022-04-20T00:05:27Z

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

Test Type	JDK	Job ID	Logs
conda	openjdk8	38959.5	logs

gatk-bot · 2022-04-20T10:37:58Z

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

Test Type	JDK	Job ID	Logs
conda	openjdk8	38961.5	logs

mcovarr · 2022-04-20T11:38:44Z

...va/org/broadinstitute/hellbender/tools/gvs/common/WeightedSplitIntervalsIntegrationTest.java

@@ -62,7 +54,6 @@ public void testNoLossRealisticWgs() {

        final int scatterCount = 100;
        final File outputDir = createTempDir("output");
-        final Interval interval = new Interval("chr20", 1000000, 2000000);


this variable in an existing test wasn't being used which may or may not be concerning?

Probably just a cut and paste from testNoLossSimple

Good spot, I think you're probably right.

gatk-bot · 2022-04-20T12:02:27Z

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

Test Type	JDK	Job ID	Logs
conda	openjdk8	38963.5	logs

gatk-bot · 2022-04-20T12:08:22Z

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

Test Type	JDK	Job ID	Logs
conda	openjdk8	38965.5	logs

gbggrant · 2022-04-20T13:20:26Z

...va/org/broadinstitute/hellbender/tools/gvs/common/WeightedSplitIntervalsIntegrationTest.java

@@ -62,7 +54,6 @@ public void testNoLossRealisticWgs() {

        final int scatterCount = 100;
        final File outputDir = createTempDir("output");
-        final Interval interval = new Interval("chr20", 1000000, 2000000);


Probably just a cut and paste from testNoLossSimple

gbggrant · 2022-04-20T13:23:56Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/WeightedInterval.java

@@ -61,15 +61,15 @@ public WeightedInterval[] split(int basesInFirstInterval) {
                this.getStart(),
                this.getStart() + basesInFirstInterval - 1,
                this.isNegativeStrand(),
-                this.getName() + "-1", // ensure names are unique
+                this.getName() == null ? null : this.getName() + "-1", // ensure non-null names are unique


Would it make sense to give them names? Not necessary, but looks kind of odd without them.

gbggrant · 2022-04-20T13:26:41Z

...va/org/broadinstitute/hellbender/tools/gvs/common/WeightedSplitIntervalsIntegrationTest.java

@@ -88,6 +79,41 @@ public void testNoLossRealisticWgs() {

    }

+    @Test
+    public void testHandleZeroBasesToTake() {


If you ran this test with the pre-fix code does it fail?

Yes that's how I discovered all the scatter widths that exercise the condition. Much easier to find if the code under test throws an exception, no need to set a breakpoint. 🙂

kcibul

nice work Miguel!

kcibul · 2022-04-22T12:57:58Z

src/main/java/org/broadinstitute/hellbender/tools/gvs/common/WeightedSplitIntervals.java

+
+                if (basesToTake == 0) {
+                    // We can't add any more bases to the current interval list so put this interval back.
+                    iter.pushback(wi);


looks great

mcovarr commented Apr 19, 2022

View reviewed changes

mcovarr commented Apr 20, 2022

View reviewed changes

mcovarr requested review from rsasch, RoriCremer and gbggrant April 20, 2022 11:39

RoriCremer approved these changes Apr 20, 2022

View reviewed changes

gbggrant approved these changes Apr 20, 2022

View reviewed changes

mcovarr changed the title ~~WeightedSplitInterval fixes [VS-384]~~ WeightedSplitInterval fixes [VS-384] [VS-332] Apr 20, 2022

kcibul approved these changes Apr 22, 2022

View reviewed changes

mcovarr and others added 8 commits April 22, 2022 09:35

fix handling of null names in split intervals

e84990c

not an experimental feature!

592e687

WeightedSplitIntervals edge case fix

2f461a5

name tests

4a80bc1

zero basesToTake integration test

1eee19f

words

699ecf2

more focused test (#7803)

ef51fc9

remove "magical" test in favor of more focused test

553417e

mcovarr force-pushed the weighted_split_interval_edge_case_fix branch from c7a7f8b to 553417e Compare April 22, 2022 13:36

restore @ExperimentalFeature

ab8e4a2

mcovarr merged commit 614a0f7 into ah_var_store Apr 25, 2022

mcovarr deleted the weighted_split_interval_edge_case_fix branch April 25, 2022 13:05

This was referenced Mar 17, 2023

lb merge gvs branch #8248

Closed

testing something, please ignore #8251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WeightedSplitInterval fixes [VS-384] [VS-332] #7795

WeightedSplitInterval fixes [VS-384] [VS-332] #7795

mcovarr commented Apr 19, 2022 •

edited

Loading

mcovarr Apr 19, 2022 •

edited

Loading

RoriCremer Apr 20, 2022

mcovarr Apr 20, 2022

gbggrant Apr 20, 2022

mcovarr Apr 20, 2022

kcibul Apr 22, 2022

mcovarr Apr 19, 2022 •

edited

Loading

kcibul Apr 22, 2022

mcovarr Apr 19, 2022

kcibul Apr 22, 2022

mcovarr Apr 22, 2022

kcibul Apr 22, 2022

gatk-bot commented Apr 20, 2022

gatk-bot commented Apr 20, 2022

mcovarr Apr 20, 2022 •

edited

Loading

gbggrant Apr 20, 2022

mcovarr Apr 20, 2022

gatk-bot commented Apr 20, 2022

gatk-bot commented Apr 20, 2022

gbggrant Apr 20, 2022

gbggrant Apr 20, 2022

gbggrant Apr 20, 2022

mcovarr Apr 20, 2022

kcibul left a comment

kcibul Apr 22, 2022

WeightedSplitInterval fixes [VS-384] [VS-332] #7795

WeightedSplitInterval fixes [VS-384] [VS-332] #7795

Conversation

mcovarr commented Apr 19, 2022 • edited Loading

mcovarr Apr 19, 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

mcovarr Apr 19, 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

gatk-bot commented Apr 20, 2022

gatk-bot commented Apr 20, 2022

mcovarr Apr 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatk-bot commented Apr 20, 2022

gatk-bot commented Apr 20, 2022

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

kcibul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcovarr commented Apr 19, 2022 •

edited

Loading

mcovarr Apr 19, 2022 •

edited

Loading

mcovarr Apr 19, 2022 •

edited

Loading

mcovarr Apr 20, 2022 •

edited

Loading