Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a bug where overlapping reads in subsequent regions can have invalid base qualities #6943

Merged
merged 3 commits into from
Nov 24, 2020
Merged

Fixed a bug where overlapping reads in subsequent regions can have invalid base qualities #6943

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c38242
Added a hack involving transient attributes to make sure everything g…
jamesemery Nov 6, 2020
a55a266
no more transient attributes...
jamesemery Nov 6, 2020
62ddfcf
responding to comments by clarifying the tests
jamesemery Nov 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...org/broadinstitute/hellbender/tools/walkers/haplotypecaller/AssemblyBasedCallerUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ public static void finalizeRegion(final AssemblyRegion region,
final List<GATKRead> readsToUse = region.getReads().stream()
// TODO unclipping soft clips may introduce bases that aren't in the extended region if the unclipped bases
// TODO include a deletion w.r.t. the reference. We must remove kmers that occur before the reference haplotype start
// NOTE: this flag is used to indicate if the read in question was modified by the following clipping operations, which
// themselves make copies of the reads only if they actually adjust anything. For safety sake we want to ensure that every
// read being handed to the overlapping pair code and caller are copied so subsequent regions don't see altered reads.
.map(read -> {read.setTransientAttribute("Original",read); return read;})
.map(read -> dontUseSoftClippedBases || ! ReadUtils.hasWellDefinedFragmentSize(read) ?
ReadClipper.hardClipSoftClippedBases(read) : ReadClipper.revertSoftClippedBases(read))
.map(read -> softClipLowQualityEnds ? ReadClipper.softClipLowQualEnds(read, minTailQualityToUse) :
Expand All @@ -114,6 +118,9 @@ public static void finalizeRegion(final AssemblyRegion region,
.filter(read -> !read.isEmpty() && read.getCigar().getReadLength() > 0)
.map(read -> ReadClipper.hardClipToRegion(read, region.getPaddedSpan().getStart(), region.getPaddedSpan().getEnd() ))
.filter(read -> read.getStart() <= read.getEnd() && read.getLength() > 0 && read.overlaps(region.getPaddedSpan()))
// The transient attribute is preserved across copy operations and all of the previous alterations make copies, this simple ensures
// that any reads that have not been copied along the way are copied here for safety.
.map(read -> (read.getTransientAttribute("Original") != read? read : read.copy()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of relying on transient attributes (which, in my opinion, is brittle and a recipe for future problems), compare the memory address of the read pre/post clipping, and make a copy only if the address hasn't changed, as in the original patch in #4926. Rewrite this method to use a loop instead of streaming, if necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have rewritten it as a loop at the expense of angering the Aesthetic gods. Can you re-review?

.sorted(new ReadCoordinateComparator(readsHeader)) // TODO: sort may be unnecessary here
.collect(Collectors.toList());

Expand Down
2 changes: 1 addition & 1 deletion ...dinstitute/hellbender/tools/walkers/haplotypecaller/AssemblyBasedCallerUtilsUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public void testfinalizeRegion() {
activeRegion.addAll(reads);
SampleList sampleList = SampleList.singletonSampleList("tumor");
Byte minbq = 9;
AssemblyBasedCallerUtils.finalizeRegion(activeRegion, false, false, minbq, header, sampleList, false, false);
AssemblyBasedCallerUtils.finalizeRegion(activeRegion, false, false, minbq, header, sampleList, true, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that the modified version of this test fails without the copy() call you added above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already tested it. It does fail without the above fix. the problem is that when we added correctOverlappingBaseQualities as an argument to finalizeRegion() it was mistakenly set to false for this method, which means this test was doing nothing and thus we missed that the last round of refactoring broke the assertion in this test by allowing un-coppied base qualities to be adjusted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment then mentioning that that boolean argument is critical for the test to be meaningful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@droazen I added comments better explaining how the test works. Is this okay to merge?


// make sure reads are not changed due to finalizeRegion()
Assert.assertTrue(reads.get(0).convertToSAMRecord(header).equals(orgRead0));
Expand Down