Fix for mdtag issues with insertions #748
Conversation
| cigarIdx += 1 | ||
| cigarOperatorIndex = 0 | ||
| } | ||
| case _ => { | ||
| cigarIdx += 1 |
There was a problem hiding this comment.
is the cigarOperatorIndex = 0 not necessary here?
mind commenting these last two cases a little bit? I assume the main interesting cigar operator being passed over here is I/insertion, which MDTags don't care about... any other cases being handled here worth noting?
There was a problem hiding this comment.
cigarOperatorIndex = 0 is not necessary, it shouldn't be updated, but just added it for clarity.
The last case is insertion or clipped bases
There was a problem hiding this comment.
So it doesn't matter whether you set cigarOperatorIndex = 0 here or not? That's also confusing to me, I don't fully have my head around this.
Is the second-to-last case one that you specifically added for Ns? Is that the main focus there?
Could you add comments clarifying which operators you expect to hit each of these last two? At this point in the match I'm struggling to keep in my head what's left.
There was a problem hiding this comment.
Yes the second to last case is for N
| assert(tag.toString === "2A4A2") | ||
| } | ||
|
|
||
| test("Get correct matches for mdtag with intron and deletion between mismatches") { |
There was a problem hiding this comment.
hm, copy mcpastington? same test as above, different name?
There was a problem hiding this comment.
Yes! I should was trying to think of more cases, but didn't add the test actually
|
added some of my nits in hammerlab#2 including the check that the number of deleted bases is consistent between cigar and MDTag. thoughts on some test cases verifying that IAEs are thrown when the various assumptions are broken? |
|
@ryan-williams I fixed the dupe test and added a test case for the exception you added (mdtag and cigar disagreeing on deleted bases) I also made the match statement explicit for the operators they were catching re: |
|
cool, thanks @arahuja. LGTM once tests pass. Last complete build failed due to needing |
|
Ah ok, good to know, I didn't realize this wasn't auto-formatting, I'll run that, squash etc. |
|
LGTM as well. |
Fix for mdtag issues with insertions
Sorry all, looks the code in #727 contains a few errors when it came to parsing mismatches mixed with insertions. I added a quite a few tests to cover this case and other complex mdtags.
If you think there are interesting cases not covered in the new tests now let me know!
Thanks!