fix 4649

[SHuang-Broad]

fix #4649

The cause of the exception is a new edge case that was not imagined when the reference region segmenting logic was initially written.
It is now covered, with updated tests.

[codecov-io]

Codecov Report

Merging #4677 into master will increase coverage by 0.03%.
The diff coverage is 90.476%.

@@              Coverage Diff               @@
##              master     #4677      +/-   ##
==============================================
+ Coverage     79.894%   79.924%   +0.03%     
+ Complexity     17388     17348      -40     
==============================================
  Files           1080      1074       -6     
  Lines          63091     62942     -149     
  Branches       10180     10186       +6     
==============================================
- Hits           50406     50306     -100     
+ Misses          8701      8652      -49     
  Partials        3984      3984

Impacted Files	Coverage Δ	Complexity Δ
...ry/inference/CpxVariantInducingAssemblyContig.java	84.141% <100%> (+0.963%)	27 <3> (+3)	⬆️
...y/inference/CpxVariantCanonicalRepresentation.java	78.992% <71.429%> (+0.022%)	52 <0> (+2)	⬆️
...transforms/markduplicates/MarkDuplicatesSpark.java	90.909% <0%> (-4.213%)	9% <0%> (-6%)
...hellbender/tools/walkers/mutect/Mutect2Engine.java	87.654% <0%> (-3.39%)	50% <0%> (+1%)
...tools/spark/validation/CompareDuplicatesSpark.java	82.927% <0%> (-1.518%)	24% <0%> (ø)
...forms/markduplicates/MarkDuplicatesSparkUtils.java	89.5% <0%> (-1.083%)	58% <0%> (-9%)
...ellbender/tools/walkers/vqsr/CNNScoreVariants.java	74.057% <0%> (-0.829%)	40% <0%> (-1%)
...r/engine/filters/ReadGroupBlackListReadFilter.java	83.333% <0%> (-0.303%)	17% <0%> (ø)
...lkers/ReferenceConfidenceVariantContextMerger.java	94.979% <0%> (-0.278%)	69% <0%> (-2%)
...stitute/hellbender/tools/walkers/CombineGVCFs.java	93.75% <0%> (-0.25%)	60% <0%> (-2%)
... and 35 more

[TedBrookings]

Assuming I'm correct about the above being a typo, you might decrease scope for this kind of error be refactoring thusly:

final int start = rightBoundary.getStart();
final int end = leftBoundary.getEnd();
final int segmentLength = end - start;
if( segmentLength >= 1) {
    final SimpleInterval newSegment = new SimpleInterval(eventPrimaryChromosome, start, end);
    if(segmentLength > 1 || twoBaseBoundaries.contains(newSegment)) {
        segments.add(newSegment);
    }
}

[SHuang-Broad]

I see what you mean, but like I replied in the other place, it's a "point" location so won't matter in this case.
And the suggestion you made is good, except that end is always >= start because it is from the "left" (I know, so many mind-numbing details...)

[TedBrookings]

Should the new SimpleInterval use leftBoundary.getEnd() ?

[SHuang-Broad]

It actually doesn't matter, because the boundaries are 1bp "point" locations.
I'll make it clear in the docs.

[SHuang-Broad]

also changed to getEnd()

[TedBrookings]

Should be 2 * contigWithFineTunedAlignments.getAlignments().size(), since you get up to two SimpleIntervals per alignment.

[SHuang-Broad]

yep and done as suggested

[TedBrookings]

I found one typo that must be fixed, however it is quite simple to fix so I'll just approve now.

The test code is nigh impenetrable (to me). Partly that's because I'm just looking at this part of the pipeline for the first time, partly it's because you're addressing an inherently complex problem, and partly it's all those "manually curated values". I have no idea if it's feasible or totally crazy, but it might be good to have an adversarial function that generates problems from known solutions rather than doing it by hand.

[SHuang-Broad]

It's always difficult to come up with test data for these complex events.

What I usually do is to create test data on what I know should work, including imaginable edge cases, and accumulate more edge cases as we run into them.

Can you expand a little on what you mean by

to have an adversarial function that generates problems from known solutions rather than doing it by hand

[TedBrookings]

Like I said, I don't know enough to say if this approach is feasible, but sometimes it's possible to test an algorithm with a problem-generating function. This works well when it's easy to generate self-consistent solutions and test data consistent with a given solution. I was able to do this for some of the interval overlap functions on the python side of my code. Other methods were too complicated (for me to figure out anyway).

Let's say you have an algorithm function f_algo that you want to test. You design a function test_data_factory that takes as input either a known solution, or a set of parameters from which it can easily generate a known solution (by a route that is logically distinct from the way f_algo works). Then f_tester generates and returns test_data. The flow looks like

// get either random test solutions or some distinct set that tests particular edge cases
test_params = get_test_params();
test_solution = easy_solution_from_params(test_params);

test_data = test_data_factory(test_params, test_solution);
assert f_algo(test_data) == test_solution;

In some sense you've already done that, but you've personally stepped in as test_data_factory(), and I'm wondering if it would be possible to automate that to get rid of all those numbers in the test code.

[SHuang-Broad]

Hmm, I see what you mean.
It’s an interesting idea, and I think Valentin does that for some CIGAR related tests; but I think it is difficult to apply in our case without having a “simulator”. I’ll keep this in mind.