-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added conditional In Text Search for Optimum Search Schemes (Hamming Distance) #2342
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just a few minor comments :)
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you replace this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not set to zero, what else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added (2nd) ?
{ | ||
for (OptimalSearch<nbrBlocks> & s : ss) | ||
{ | ||
std::vector<uint32_t> chronBL(s.pi.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation 💅
This seems rather recent, but we are in feature freeze for SeqAn2. Can you instead work on this for SeqAn3? |
No description provided.