banded alignment implementation in optimised implementation #1874

rrahn · 2020-06-04T21:58:15Z

Adds banded alignment implementation.

~~Blocked by #1873~~

marehr

This was not so fun to review. Way to many things in one PR.

include/seqan3/range/views/drop.hpp

include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion.hpp

include/seqan3/alignment/matrix/detail/score_matrix_single_column.hpp

include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion_banded.hpp

include/seqan3/alignment/pairwise/detail/pairwise_alignment_algorithm_banded.hpp

test/unit/alignment/pairwise/global_affine_banded_test.cpp

marehr · 2020-06-07T21:47:01Z

header build failed:

[ 34%] Building CXX object CMakeFiles/seqan3_header_test--seqan3-alignment-pairwise-detail-policy_affine_gap_recursion-2.dir/seqan3_header_test_files/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion-2.cpp.o
In file included from /home/travis/build/seqan/seqan3-build/seqan3_header_test_files/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion_banded-2.cpp:2:0:
/home/travis/build/seqan/seqan3/include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion_banded.hpp:31:81: error: expected template-name before ‘<’ token
 class policy_affine_gap_recursion_banded : protected policy_affine_gap_recursion<alignment_configuration_t>
                                                                                 ^
/home/travis/build/seqan/seqan3/include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion_banded.hpp:31:81: error: expected ‘{’ before ‘<’ token
/home/travis/build/seqan/seqan3/include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion_banded.hpp:102:2: error: extra ‘;’ [-Werror=pedantic]
 };
  ^

rrahn · 2020-06-08T14:29:32Z

This was not so fun to review. Way to many things in one PR.

Well I fogot to mention that this is rebased on the other PR. But it is marked as dependent. Since you reviewed both, I assume you just skipped the replicated commits?

rrahn · 2020-06-08T14:29:59Z

header build failed:

I know. I wanted to fix it when I am working on it again.

include/seqan3/alignment/pairwise/detail/pairwise_alignment_algorithm_banded.hpp

rrahn · 2020-06-11T23:09:55Z

@marehr can you give it another round?

marehr

@marehr can you give it another round?

Of course :)

Only things left :) 🎉

include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion.hpp

include/seqan3/alignment/pairwise/detail/pairwise_alignment_algorithm_banded.hpp

marehr · 2020-06-12T12:32:25Z

include/seqan3/alignment/pairwise/detail/pairwise_alignment_algorithm_banded.hpp

+    template <std::ranges::input_range alignment_column_t,
+              std::ranges::input_range cell_index_column_t,
+              semialphabet sequence1_value_t,
+              std::ranges::input_range sequence2_t>


@smehringer These are examples of seqan3::sequence, since you asked me :)

include/seqan3/range/views/drop.hpp

include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion_banded.hpp

marehr

Thank you so much :)

codecov · 2020-06-14T02:39:47Z

Codecov Report

Merging #1874 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1874      +/-   ##
==========================================
- Coverage   97.76%   97.76%   -0.01%     
==========================================
  Files         253      255       +2     
  Lines        9560     9643      +83     
==========================================
+ Hits         9346     9427      +81     
- Misses        214      216       +2

Impacted Files	Coverage Δ
...t/pairwise/detail/pairwise_alignment_algorithm.hpp	`100.00% <ø> (ø)`
...nment/matrix/detail/score_matrix_single_column.hpp	`100.00% <100.00%> (ø)`
...qan3/alignment/pairwise/alignment_configurator.hpp	`100.00% <100.00%> (ø)`
...ise/detail/pairwise_alignment_algorithm_banded.hpp	`100.00% <100.00%> (ø)`
...nt/pairwise/detail/policy_affine_gap_recursion.hpp	`100.00% <100.00%> (ø)`
...wise/detail/policy_affine_gap_recursion_banded.hpp	`100.00% <100.00%> (ø)`
include/seqan3/range/views/drop.hpp	`100.00% <100.00%> (ø)`
.../seqan3/alignment/pairwise/alignment_algorithm.hpp	`98.50% <0.00%> (-1.50%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f799c0...b404eaa. Read the comment docs.

smehringer

Only minor things, otherwise LGTM

smehringer · 2020-06-16T05:57:24Z

include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion.hpp

+    score_type lowest_viable_score() const noexcept
+    {
+        assert(m_gap_open_score <= 0 && m_gap_extension_score <= 0);
+        return std::numeric_limits<score_type>::lowest() - (m_gap_open_score + m_gap_extension_score);


Maybe a dumb question, but doesn't this only protect against an underflow once? Can it not happen that I try to subtract the gap penalty multiple times?

That can only happen if the score would overflow/underflow in general because the score range is too small. But in this case we simply say that there is undefined behaviour. What we can say is, however, what would be the minimal viable score such that we do not underflow when subtracting a value from this. This has two effects: In the banded case I spare one additional case to consider when computing the score of the last cell of the banded column. Later in the local vectorised score I can choose this score as the new 0, such that I can use as much of the signed score range as possible without using saturated integer arithmetic which is not supported on all vector extensions. Since we use a maximisation objective, we will never propagate the small value to a new cell. Unless of course, the score range is not sufficiently large (see first point).

smehringer · 2020-06-16T06:02:22Z

include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion.hpp

@@ -214,8 +214,8 @@ class policy_affine_gap_recursion
     */
    score_type lowest_viable_score() const noexcept
    {
-        assert(m_gap_open_score <= 0 && m_gap_extension_score <= 0);
-        return std::numeric_limits<score_type>::lowest() - (m_gap_open_score + m_gap_extension_score);
+        assert(gap_open_score <= 0 && gap_extension_score <= 0);


I don't see the name change in the actual member variable in this commit ([MISC] Remove m_ prefix from affine internal variables.)?

Probably, because of some rebasing issues.

I will resolve the commit once I do the final rebase.

smehringer · 2020-06-16T06:07:03Z

include/seqan3/alignment/pairwise/detail/policy_affine_gap_recursion_banded.hpp

+    {}
+    //!\}
+
+    /*!\brief Initialises the first cell of a banded column that does not start in the first row of the matrix.


it says here ... not start in the first row but the function is called initialise_band_first_row.... Maybe a copy and paste error?

Not an error but maybe I rename the function to make it more clear.

include/seqan3/alignment/pairwise/detail/pairwise_alignment_algorithm_banded.hpp

smehringer · 2020-06-16T06:11:02Z

include/seqan3/alignment/pairwise/detail/pairwise_alignment_algorithm_banded.hpp

+    //!\}
+
+protected:
+    /*!\brief Checks whether the band given the sequence sizes is valid.


Suggested change

/*!\brief Checks whether the band given the sequence sizes is valid.

/*!\brief Checks whether the band, given the sequence sizes, is valid.

smehringer · 2020-06-16T06:15:58Z

include/seqan3/alignment/pairwise/detail/pairwise_alignment_algorithm_banded.hpp

+
+        this->track_last_column_cell(*alignment_column_it, *cell_index_column_it);
+
+        for ([[maybe_unused]] auto && unused : sequence2 | views::slice(first_row_index, row_size))


this has no performance regression compared to a simple for (size_t i = first_row_index; i < first_row_index + row_size; ++i)?

It is not about performance. It is about beautiful code ;)

beauty lies in the eye of the beholder :)

smehringer

There are merge conflicts..

* Makes the affine gap policy class protected.

…cted.

rrahn requested a review from marehr June 4, 2020 21:58

rrahn force-pushed the alignment/banded_implementation branch from abc9c67 to 2acfcdc Compare June 5, 2020 07:11

marehr requested changes Jun 7, 2020

View reviewed changes

rrahn commented Jun 8, 2020

View reviewed changes

include/seqan3/alignment/pairwise/detail/pairwise_alignment_algorithm_banded.hpp Outdated Show resolved Hide resolved

rrahn requested a review from marehr June 8, 2020 15:51

rrahn force-pushed the alignment/banded_implementation branch 2 times, most recently from 5eb2f6d to 9ec6399 Compare June 11, 2020 23:07

marehr requested changes Jun 12, 2020

View reviewed changes

rrahn requested a review from marehr June 12, 2020 13:57

marehr approved these changes Jun 12, 2020

View reviewed changes

marehr requested a review from smehringer June 12, 2020 16:46

rrahn force-pushed the alignment/banded_implementation branch 2 times, most recently from e89c177 to 743cb49 Compare June 13, 2020 23:52

smehringer requested changes Jun 16, 2020

View reviewed changes

rrahn requested a review from smehringer June 19, 2020 15:01

smehringer approved these changes Jun 22, 2020

View reviewed changes

rrahn added 9 commits June 23, 2020 16:34

[MISC] Fix some signed/unsigned warning in drop view.

bb29c47

[MISC] Add lowest viable score function

67988d6

* Makes the affine gap policy class protected.

[MISC] Allow the score matrix to be initialised with a specific value.

8d7cadc

[MISC] Make the type definitons of pairwise alignment algorithm prote…

a48f3cd

…cted.

[DOC] Fix documentation in pairwise alignment algorithm.

9df1ab4

[DOC] Fix brief of comute_inner_cell function.

6df0174

[FEATURE] Adds affine gap recursion policy for the banded alignment.

12682da

[FEATURE] Add banded alignment algorithm.

9129319

[MISC] Enable the banded alignment computing only the score.

e508f23

[TEST] Add more test cases for the banded global alignment.

b404eaa

rrahn force-pushed the alignment/banded_implementation branch from b82b352 to b404eaa Compare June 23, 2020 16:02

rrahn merged commit 2a9cf54 into seqan:master Jun 23, 2020

rrahn mentioned this pull request Jun 25, 2020

alingment improvement refactorisation - banded alignment seqan/product_backlog#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

banded alignment implementation in optimised implementation #1874

banded alignment implementation in optimised implementation #1874

rrahn commented Jun 4, 2020 •

edited

Loading

marehr left a comment

marehr commented Jun 7, 2020

rrahn commented Jun 8, 2020

rrahn commented Jun 8, 2020

rrahn commented Jun 11, 2020

marehr left a comment

marehr Jun 12, 2020

marehr left a comment

codecov bot commented Jun 14, 2020 •

edited

Loading

smehringer left a comment

smehringer Jun 16, 2020

rrahn Jun 19, 2020 •

edited

Loading

smehringer Jun 16, 2020

rrahn Jun 19, 2020

rrahn Jun 19, 2020

smehringer Jun 16, 2020

rrahn Jun 19, 2020 •

edited

Loading

smehringer Jun 16, 2020

smehringer Jun 16, 2020

rrahn Jun 19, 2020

smehringer Jun 22, 2020

smehringer left a comment

	/*!\brief Checks whether the band given the sequence sizes is valid.
	/*!\brief Checks whether the band, given the sequence sizes, is valid.


		this->track_last_column_cell(alignment_column_it, cell_index_column_it);

		for ([[maybe_unused]] auto && unused : sequence2 \| views::slice(first_row_index, row_size))

banded alignment implementation in optimised implementation #1874

banded alignment implementation in optimised implementation #1874

Conversation

rrahn commented Jun 4, 2020 • edited Loading

marehr left a comment

Choose a reason for hiding this comment

marehr commented Jun 7, 2020

rrahn commented Jun 8, 2020

rrahn commented Jun 8, 2020

rrahn commented Jun 11, 2020

marehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marehr left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 14, 2020 • edited Loading

Codecov Report

smehringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smehringer left a comment

Choose a reason for hiding this comment

rrahn commented Jun 4, 2020 •

edited

Loading

codecov bot commented Jun 14, 2020 •

edited

Loading

rrahn Jun 19, 2020 •

edited

Loading

rrahn Jun 19, 2020 •

edited

Loading