-
Notifications
You must be signed in to change notification settings - Fork 80
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
Feature/compute vectorised score for global alignment #1379
Feature/compute vectorised score for global alignment #1379
Conversation
532b9d9
to
70f8ca5
Compare
Codecov Report
@@ Coverage Diff @@
## master #1379 +/- ##
==========================================
+ Coverage 97.56% 97.58% +0.01%
==========================================
Files 231 232 +1
Lines 8755 8822 +67
==========================================
+ Hits 8542 8609 +67
Misses 213 213
Continue to review full report at Codecov.
|
We wanted to keep PRs small... I hope this is not an urgent one, I'll need 2-3 days. |
I tried to make the commits as small as possible. The biggest commit however could not be reduced without introducing compile errors or failing tests. I did not want to open PRs for one-line commits and then manage all the PRs with several rebases. That is not bearable either. I removed larger components already. Also note that only last 9 commits are relevant. In total it was a 500 line change or something. I hope that is 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.
Reviewed commits 11-14
include/seqan3/alignment/pairwise/policy/simd_affine_gap_policy.hpp
Outdated
Show resolved
Hide resolved
include/seqan3/alignment/pairwise/policy/simd_affine_gap_policy.hpp
Outdated
Show resolved
Hide resolved
include/seqan3/alignment/pairwise/policy/simd_affine_gap_policy.hpp
Outdated
Show resolved
Hide resolved
include/seqan3/alignment/pairwise/policy/simd_affine_gap_policy.hpp
Outdated
Show resolved
Hide resolved
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.
This is the review for the remaining commits 15-20. As discussed I went through my comments afterwards and applied the suggestion feature where possible.
not sure why suddenly the osx builds fail. But maybe this is related to catalina issue with the aligned allocator. Waiting for #1362 to test if the error still occurs. |
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.
Thank you! Looks good now.
f847f5a
to
f208b50
Compare
@smehringer I cleaned the commits now. Only the very first commit is not necessary anymore. |
On mac os build failure is probably related to #1362 |
test/unit/alignment/pairwise/pairwise_alignment_collection_test_template.hpp
Outdated
Show resolved
Hide resolved
test/unit/alignment/pairwise/pairwise_alignment_collection_test_template.hpp
Outdated
Show resolved
Hide resolved
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 forgot: please fix the commit history.
And what about a note in the Changelog?
Well I wanted to add this when the stuff is complete with the various configurations. |
d35d89d
to
01042a5
Compare
@smehringer polite ping |
I guess it cannot be merged yet since it still depends on another PR |
yes. right. I forgot about that as well. Too long ago |
…ned in alignment configuration.
Not all tests can work at the moment with the vectorised alignment code and must be disabled for now. For some test cases these can be reenabled when the vectorisation is implemented.
* Tests dna sequences of same length. * Only score and back coordinate can be computed.
01042a5
to
940d824
Compare
Fixes #1351
Implements the initial version of vectorised alignments.
🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉
Depends on PR #1378 and issue #1375
Accordingly, only the last 9 commits are relevant.
On macos (catalina) the CI breaks because of the allocator bug. This should be fixed once #1362 is merged.