-
Notifications
You must be signed in to change notification settings - Fork 20
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
[fix] alignment bug #222
[fix] alignment bug #222
Conversation
ecccc41
to
742533a
Compare
src/search_misc.hpp
Outdated
@@ -46,7 +46,7 @@ struct QueryException : public std::runtime_error | |||
inline int _bandSize(uint64_t const seqLength) |
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.
Should the return type here also be int64_t
?
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.
Done. I don't think this affects anything though.
src/search_algo.hpp
Outdated
// pairwise "swallow" from right to left | ||
for (auto it = matches.rbegin(); it < matches.rend() - 1; ++it) | ||
{ | ||
Match & l = *it; |
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.
Given that the comment says from right to left, I understood it in a way that we are still "looking" at the vector the same way as before. However, now that we are backwards iterating technically l
is to the right of r
in the sense of that comment and it confused me naming-wise when I tried to understand what is happening (because it does not match the l
and r
in the loop before order-wise). Can we find a different way of naming this that is more intuitive?
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.
That's a good point. I have switched r
and l
. Is that OK?
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.
yes, thanks!
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.
LGTM
tests still need fixing