An eachkmer iterator fix #8
Conversation
|
Good catch! I haven't noticed this problem |
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
=========================================
- Coverage 83.25% 82.86% -0.4%
=========================================
Files 41 41
Lines 2885 2906 +21
=========================================
+ Hits 2402 2408 +6
- Misses 483 498 +15
Continue to review full report at Codecov.
|
src/eachkmer.jl
Outdated
|
|
||
| # faster path: return the next overlapping kmer if possible | ||
| if it.step < k && pos + k - 1 ≤ endof(it.seq) | ||
| offset = k - it.step | ||
| if it.step == 1 | ||
| nt = inbounds_getindex(it.seq, pos+offset) | ||
| kmer = kmer << 2 | trailing_zeros(nt) | ||
| has_ambiguous |= isambiguous(nt) | ||
| ambig_or_gap |= (isambiguous(nt) || isgap(nt)) |
There was a problem hiding this comment.
Why not iscertain(nt)? A quick benchmark on my machine suggests no performance degradation by switching isambiguous with iscertain.
There was a problem hiding this comment.
Also, adding test cases to make sure gap skipping is nice.
There was a problem hiding this comment.
Good catch! I should have seen iscertain was applicable here, I used it instead of isambiguous and isgap in the other site counting stuff I've done. I'll make the iscertain change now, and add a test case in the morning - it's too late here!
|
Ok here we go, a test explicitly checking a sequence with gaps. If this passes and is alright, I'd like to merge this fix when the checks are done. |
Eliminates multiple negation operations.
TransGirlCodes
force-pushed
the
kmer_iterator_fix
branch
from
bb9a7cc
to
aea99be
Compare
|
👍 CI failure on Linux would not be a problem. |
@bicycle1885 This edits some lines of the eachkmer iterator, so as well as checking for ambigs, it also checks for gaps. Hope this is ok, if it is, the use of
each_kmer_implin some specific iterator I can write will actually allow me to do a lot of my work without the need for a whole newCodontype, which closes the issue and is a huge timesaving win!