Added conditional In Text Search for Optimum Search Schemes (Hamming Distance) #2342
Conversation
There was a problem hiding this comment.
Thanks for the PR! As mentioned in the comments I think we can significantly reduce redundant code.
Please also adjust the coding styles to the general seqan2 coding guidelines resp. the style used in the corresponding file (see https://seqan.readthedocs.io/en/master/Infrastructure/Contribute/StyleCpp.html#infra-contribute-style-cpp )
Mostly indents, curly braces start on a new line. spaces after for/if keyword
| @@ -438,7 +553,8 @@ inline void _optimalSearchSchemeExact(TDelegate & delegate, | |||
| TDir const & /**/, | |||
| TDistanceTag const & /**/) | |||
| { | |||
| bool goToRight2 = (blockIndex < s.pi.size() - 1) && s.pi[blockIndex + 1] > s.pi[blockIndex]; | |||
| bool goToRight2 = (blockIndex < s.pi.size() - 1) ? s.pi[blockIndex + 1] > s.pi[blockIndex] : | |||
There was a problem hiding this comment.
why did you replace this?
There was a problem hiding this comment.
It is not really necessary, since does not have an impact on the current version of the code, but if we are in the last block (of the search) and going to right, goToRight2 value was false.
| { | ||
| for (OptimalSearch<nbrBlocks> & s : ss) | ||
| { | ||
| std::vector<uint32_t> chronBL(s.pi.size()); |
There was a problem hiding this comment.
replace std::vector by std::array of size nbrBlocks. Thus you enforce that chronBL lands on the stack and not the heap which improves performance
There was a problem hiding this comment.
If you do add a debug assert that nbrBlocks == s.pi.size() (if this is correct in this context)
There was a problem hiding this comment.
Most of this are questions or minor comments. I'll let @cpockrandt explain to me what happens here and then I can review the semantics.
Please also extend the tests such that the new behaviour is tested.
|
|
||
| std::array<uint32_t, N> blocklength; // cumulated length of the blocks in Search Scheme order | ||
| //NOTE (svnbgnk) added additional information about search schemes depending on the read length | ||
| //These values are not set to Zero during the creation of Optimal Search Schemes |
There was a problem hiding this comment.
If not set to zero, what else?
There was a problem hiding this comment.
I think they are left empty.
@cpockrandt Maybe I should add Zeroes to the OptimalSearchSchemes structs (for blockStarts and blockEnds)?
| @@ -80,7 +84,7 @@ struct OptimalSearchSchemes<0, 2, TVoidType> | |||
| { | |||
| static constexpr std::array<OptimalSearch<4>, 3> VALUE | |||
| {{ | |||
| { {{2, 1, 3, 4}}, {{0, 0, 1, 1}}, {{0, 0, 2, 2}}, {{0, 0, 0, 0}}, 0 }, | |||
| { {{1, 2, 3, 4}}, {{0, 0, 1, 1}}, {{0, 0, 2, 2}}, {{0, 0, 0, 0}}, 0 }, | |||
There was a problem hiding this comment.
Why this change? If this is a fix to an optimal search scheme this should go into a separate PR IMPOV.
| @@ -268,6 +272,27 @@ inline void _optimalSearchSchemeSetBlockLength(std::array<OptimalSearch<nbrBlock | |||
| s.blocklength[i] = blocklength[s.pi[i]-1] + ((i > 0) ? s.blocklength[i-1] : 0); | |||
| } | |||
|
|
|||
| //NOTE (svnbngk) added new function to calculate added chronological block length in the search schemes | |||
| { | ||
| for (OptimalSearch<nbrBlocks> & s : ss) | ||
| { | ||
| std::vector<uint32_t> chronBL(s.pi.size()); |
There was a problem hiding this comment.
If you do add a debug assert that nbrBlocks == s.pi.size() (if this is correct in this context)
|
|
||
| sa_info.i2 = sa_info.i2 - needleLeftPos; | ||
| uint8_t errors2 = errors; | ||
| // iterate over each block according to search in search scheme |
|
This seems rather recent, but we are in feature freeze for SeqAn2. Can you instead work on this for SeqAn3? |
No description provided.