Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix a bug with soft clips in LeftAlignIndels #6792

Merged
merged 3 commits into from
Oct 8, 2020

Conversation

davidbenjamin
Copy link
Contributor

Closes #6765. @fleharty can you review this fix?

Copy link
Contributor

@fleharty fleharty left a comment

Choose a reason for hiding this comment

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

@davidbenjamin
I don't see any tests that have leading or trailing softclips. Maybe I missed them?
Also, we are now using hard clipping a lot more frequently too, so could you add a few tests that include hard clipped reads on both ends.

Otherwise I think this looks good.

@davidbenjamin
Copy link
Contributor Author

@fleharty See lines 789-90 in AlignmentUtilsUnitTest.java for soft clips. I will do the same thing for hard clips.

@jamesemery
Copy link
Collaborator

jamesemery commented Sep 14, 2020

@davidbenjamin Interesting. In #6634 I added a fix similar to this as now HC is holding onto bases that were clipped by the various layers of low edge and low quality end removal code in the form of soft clips. I suspect your fix might be a little more elegant than mine. Could we hold off on merging this for a bbit to save ourselves the trouble when we try to rebase?

@davidbenjamin
Copy link
Contributor Author

@fleharty It now handles hard clips.

@jamesemery Could you double-check whether this and #6634 actually interfere with one another? This PR doesn't affect HC at all. That is, it neither touches HC code nor affects its output. All it does is make an AlignmentUtils method correct for a case — leading clips adjacent to an indel — that doesn't come up in HC.

Copy link
Contributor

@fleharty fleharty left a comment

Choose a reason for hiding this comment

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

Great thanks!

@davidbenjamin
Copy link
Contributor Author

@jamesemery Should I merge this PR before Friday's release, or hold off?

@jamesemery
Copy link
Collaborator

@davidbenjamin go ahead and merge. I was conflating this code with some other code in the DRAGEN branch that also deals with Indels.

@droazen droazen merged commit 4982c2f into master Oct 8, 2020
@droazen droazen deleted the db_left_align_indels_bug branch October 8, 2020 15:56
mwalker174 pushed a commit that referenced this pull request Nov 3, 2020
Fixes an "IllegalArgumentException: the range cannot contain negative indices" error in LeftAlignIndels

Resolves #6765
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

leftalignindels: java.lang.IllegalArgumentException: the range cannot contain negative indices
4 participants