fixed error in RealignmentEngine due to inclusive vs exclusive ends #6404
Conversation
| private static SimpleInterval convertToInterval(final BwaMemAlignment alignment) { | ||
| return new SimpleInterval(Integer.toString(alignment.getRefId()), alignment.getRefStart(), alignment.getRefEnd()); | ||
| return new SimpleInterval(Integer.toString(alignment.getRefId()), alignment.getRefStart(), alignment.getRefEnd() - 1); |
There was a problem hiding this comment.
I think we need to go the other way don't we? Increase the start and leave the end alone?
There was a problem hiding this comment.
Suppose the last included position is 5.
-
Account for exclusive --> inclusive stop.
BWA mem alignment is exclusive, hence its end is 6. SimpleInterval is inclusive, so its end should be 5, which is BWA's end - 1. -
account for 0-based --> 1-based contig
Add 1 to everything, so that adding 1 to the end cancels subtracting 1, while the start is incremented by 1.
@lbergelson You're right. Thank you!
…inclusive interval ends
davidbenjamin
force-pushed
the
db_realignment_bug
branch
from
f1b4e60
to
5d33c3f
Compare
| private static SimpleInterval convertToInterval(final BwaMemAlignment alignment) { | ||
| return new SimpleInterval(Integer.toString(alignment.getRefId()), alignment.getRefStart(), alignment.getRefEnd()); | ||
| return new SimpleInterval(Integer.toString(alignment.getRefId()), alignment.getRefStart() + 1, alignment.getRefEnd()); |
There was a problem hiding this comment.
@davidbenjamin Are we sure that the contig is correct? I think alignment.getRefId() is likely to be 0 indexed, which means that you'll start with contig 0 when you want contig 1. And obviously it doesn't work for non-numbered contigs. I think what you need to be doing here is looking it up from bwaMemIndex.getReferenceContigNames().
There was a problem hiding this comment.
Oh. I see, it's using the same method to generate the overlap detectors, so that's ok.
Closes #6344, I think. @takutosato can you review this?