Misc/refactor aligned sequence builder #1237

rrahn · 2019-09-03T13:34:29Z

NOTE This PR is for discussing the current situation with the begin_ra/end_ra and the to_random_access view concept.

In General I am not very happy with doing all this additional random access semantics stuff.
Some of my general concerns:

1. The view only wraps the original type and forwards its begin_ra/end_ra members as begin/end. But the type is lost and thus it is not directly usable for using it as the original data structure. But often this is what you want. For example, the gap_decorator fulfills the aligned sequence concept and the bidirectional range concept. You can raise it to random access range as proposed originally but then you loose the aligned sequence concept. However, in case of the gap_decorator you want to be able to use the interface also with the random access iterator (which is also fixed in this PR). Then you can use iterator from a different type as regular iterators to a different interface.
2. I don't see many other places except our decorators where you would model random access even if it might not be constant time access. Thus, we are effectively preventing the efficient use case in the default version (it was fixable in the gap_decorator, but not sure if I can generalize this), which is worse then expecting that access should be constant even if it is not. I think if documented it should be ok to only stick to one begin/end and always give the weak_random_access_iterator in this case. It would be in general less confusing to use (see first point).

The view random access range is moved out into another PR together with some changes related to the gap_decorator. This PR is now rebased on this.

Blocked by #1283

codecov · 2019-09-03T21:29:11Z

Codecov Report

Merging #1237 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1237      +/-   ##
==========================================
+ Coverage   97.58%   97.61%   +0.02%     
==========================================
  Files         221      222       +1     
  Lines        8975     8999      +24     
==========================================
+ Hits         8758     8784      +26     
+ Misses        217      215       -2

Impacted Files	Coverage Δ
.../seqan3/alignment/matrix/detail/trace_iterator.hpp	`100% <ø> (ø)`	⬆️
.../alignment/matrix/detail/trace_iterator_banded.hpp	`100% <100%> (ø)`	⬆️
...nclude/seqan3/range/views/pseudo_random_access.hpp	`100% <100%> (ø)`
...qan3/alignment/matrix/detail/matrix_coordinate.hpp	`100% <100%> (ø)`	⬆️
...trix/detail/alignment_trace_matrix_full_banded.hpp	`100% <100%> (ø)`	⬆️
...ignment/matrix/detail/aligned_sequence_builder.hpp	`100% <100%> (+4.44%)`	⬆️
...ment/aligned_sequence/aligned_sequence_concept.hpp	`100% <100%> (ø)`	⬆️
...n3/alignment/matrix/detail/trace_iterator_base.hpp	`100% <100%> (ø)`	⬆️

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 ebd0790...5205342. Read the comment docs.

include/seqan3/range/views/pseudo_random_access.hpp

include/seqan3/range/decorator/gap_decorator.hpp

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

eseiler

One thing, otherwise LGTM

include/seqan3/range/views/pseudo_random_access.hpp

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

include/seqan3/range/decorator/gap_decorator.hpp

include/seqan3/range/views/pseudo_random_access.hpp

rrahn · 2019-09-12T19:03:49Z

@smehringer ping

smehringer

some tiny stuff left

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

include/seqan3/range/decorator/gap_decorator.hpp

test/unit/range/views/view_pseudo_random_access_test.cpp

smehringer · 2019-09-17T10:47:04Z

I think it also to improve it's usage. I will make this before this PR and rebase it then onto this, so this one is blocked for the time being.

Is this not blocked anymore?

rrahn · 2019-09-17T10:50:54Z

Is this not blocked anymore?

No it is not anymore. I applied the changes on top of this PR. some thing I messed up. don't wanna talk about it

include/seqan3/range/decorator/gap_decorator.hpp

test/snippet/range/views/test_back_inserter_zip.cpp

include/seqan3/core/type_traits/lazy.hpp

test/snippet/core/type_traits/is_instantiable_with.cpp

marehr

sorry for revisiting some stuff 🙇‍♂️

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

test/unit/alignment/matrix/detail/aligned_sequence_builder_test.cpp

include/seqan3/range/views/pseudo_random_access.hpp

include/seqan3/range/decorator/gap_decorator.hpp

marehr · 2019-09-18T13:05:55Z

CI is failing; https://travis-ci.org/seqan/seqan3/jobs/586123431#L5487

rrahn · 2019-10-04T11:41:35Z

@marehr I have moved the random_access things out of this PR into another PR. So this is now about the changes to the aligned sequence builder only. Everything starting at commit
569bb10 is relevant for this PR. The other changes are from #1283.

marehr · 2019-10-04T14:13:59Z

Okay, thank you I have a look at it. Just to make sure:

[FEATURE] Add is_instantiable_with unary type trait.

is the first commit that I should look at.

marehr

We are getting close :) Thank you for removing that band option, you forgot to remove some left-overs. As always some naming things.

The only thing that is currently concerning to me is the is_instantiable_with, because it does not do what it describes. But that should not be a blocker for this PR.

include/seqan3/core/type_traits/lazy.hpp

test/snippet/core/type_traits/is_instantiable_with.cpp

marehr · 2019-10-04T23:22:40Z

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

     */
-    constexpr trace_iterator_banded(matrix_iter_t const matrix_iter) noexcept : base_t{matrix_iter}
+    template <typename index_t>
+    constexpr trace_iterator_banded(matrix_iter_t const matrix_iter, column_index_type<index_t> const & pivot_column)


Okay so you now moved the offset into the trace-iterator, but why is the pivot in here? I thought the matrix provides the positions as in a full matrix. So that the trace_iterator is the same for all trace matrices. That's at least what I remembered from the design sketch that we did. I would like to discuss this in person again and review the pictures of the board that we took back then. Maybe my memory is faulty.

I need to rethink it but mainly because it avoids some additional book keeping.

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

marehr · 2019-10-05T10:32:58Z

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

-    void fill_aligned_sequence(reverse_traces_t && rev_traces,
-                               fst_aligned_t & fst_aligned,
-                               sec_aligned_t & sec_aligned) const
+    template <typename reverse_traces_t, typename fst_t, typename sec_t>


why adding here an extra template for fst_t? What difference does it make? I think fst_aligned_t & fst_aligned was completely fine, because you know your passed types.

that's true

marehr · 2019-10-05T10:34:11Z

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

        }
    }

    all_view<fst_rng_t> fst_rng; //!< A view over the first range.
    all_view<sec_rng_t> sec_rng; //!< A view over the second range.
+    std::optional<size_t> band_col_offset; //!< The offset of the column where an optional band starts.


This is now obsolete.

Suggested change

std::optional<size_t> band_col_offset; //!< The offset of the column where an optional band starts.

marehr · 2019-10-05T10:34:42Z

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

@@ -12,13 +12,16 @@

 #pragma once

+#include <optional>


This now obsolete

Suggested change

#include <optional>

marehr · 2019-10-05T10:35:41Z

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

+//!\brief Deduces the type from the passed constructor arguments.
+template <std::ranges::viewable_range fst_rng_t, std::ranges::viewable_range sec_rng_t>
+aligned_sequence_builder(fst_rng_t, sec_rng_t, std::optional<static_band>)
+    -> aligned_sequence_builder<fst_rng_t, sec_rng_t>;


remove me please, thank you!

marehr · 2019-10-05T10:36:59Z

test/unit/alignment/matrix/detail/aligned_sequence_builder_test.cpp

@@ -95,155 +97,131 @@ TYPED_TEST(aligned_sequence_builder_fixture, construction)
 TYPED_TEST(aligned_sequence_builder_fixture, build_from_2_3)
 {
    auto p = this->path(matrix_offset{row_index_type{2}, column_index_type{3}});
-    auto [begin, end, alignment] = this->builder(p);
+    auto [cut_interval_first, cut_interval_second, alignment] = this->builder(p);


Even when seeing the tests, cut_interval does not seem to be the right name.

sequence1_subrange
sequence2_subrange

would be nicer

(Oh I forgot that you can structural bind data members, thank you for reminding me :) )

As above subrange would indicate that it is in fact a subrange. But it is not. It is just the interval of the slice/subrange. I also kept the naming with the first/second. At least this matches the pair member notation in the stl.

The banded trace iterator should return the actual coordinates within the global alignment matrix and not the coordiantes of the underlying storage. This corresponding mapping is only known by the banded trace matrix.

…ges.

marehr

:)

marehr · 2019-10-08T13:40:30Z

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

-        matrix_coordinate begin; //!< The coordinate where the trimmed first and second sequence begin.
-        matrix_coordinate end; //!< The coordinate where the trimmed first and second sequence end.
-        std::pair<fst_aligned_t, sec_aligned_t> alignment; //!< The alignment over the trimmed sequences.
+        std::pair<size_t, size_t> slice_interval_first{}; //!< The slice positions of the first sequence.


marehr · 2019-10-08T13:43:18Z

test/unit/alignment/matrix/detail/aligned_sequence_builder_test.cpp

@@ -95,155 +97,131 @@ TYPED_TEST(aligned_sequence_builder_fixture, construction)
 TYPED_TEST(aligned_sequence_builder_fixture, build_from_2_3)
 {
    auto p = this->path(matrix_offset{row_index_type{2}, column_index_type{3}});
-    auto [begin, end, alignment] = this->builder(p);
+    auto [slice_interval_first, slice_interval_second, alignment] = this->builder(p);


marehr · 2019-10-09T00:40:38Z

@rrahn You messed your rebases up and introduced new files that should have been deleted:

test/unit/range/views/view_pseudo_random_access_test.cpp
include/seqan3/range/views/pseudo_random_access.hpp
test/snippet/range/views/pseudo_random_access.cpp

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch 3 times, most recently from c630f20 to 8457f9f Compare September 3, 2019 21:29

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch 2 times, most recently from 9da3eef to a6c4a57 Compare September 10, 2019 15:33

rrahn requested a review from smehringer September 10, 2019 15:33

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from a6c4a57 to c4cfc33 Compare September 10, 2019 16:25

rrahn requested review from eseiler and removed request for smehringer September 10, 2019 16:51

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from c4cfc33 to 69a9e04 Compare September 11, 2019 05:28

eseiler requested changes Sep 11, 2019

View reviewed changes

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from 69a9e04 to d3a3dba Compare September 11, 2019 11:00

rrahn requested a review from eseiler September 11, 2019 11:00

eseiler approved these changes Sep 11, 2019

View reviewed changes

include/seqan3/range/views/pseudo_random_access.hpp Show resolved Hide resolved

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from d3a3dba to 28d784b Compare September 11, 2019 11:14

rrahn requested a review from smehringer September 11, 2019 11:14

smehringer requested changes Sep 11, 2019

View reviewed changes

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from 28d784b to 0ea2159 Compare September 11, 2019 13:39

rrahn requested a review from smehringer September 11, 2019 13:39

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from 0ea2159 to c999d7d Compare September 12, 2019 19:32

rrahn mentioned this pull request Sep 12, 2019

Misc/refactor alignment algorithm final #1258

Closed

1 task

smehringer reviewed Sep 13, 2019

View reviewed changes

smehringer mentioned this pull request Sep 16, 2019

AlignedRange and gap_decorator TODOs #1103

Closed

7 tasks

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from c999d7d to 2beed90 Compare September 17, 2019 10:32

rrahn requested a review from smehringer September 17, 2019 10:33

smehringer requested changes Sep 17, 2019

View reviewed changes

marehr reviewed Sep 18, 2019

View reviewed changes

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch 6 times, most recently from fa70d6f to a3a4ca4 Compare October 2, 2019 13:04

rrahn mentioned this pull request Oct 2, 2019

[MISC] Let banded trace iterator return actual matrix coordinates. #1284

Closed

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from a3a4ca4 to dd089fd Compare October 2, 2019 21:43

rrahn requested a review from marehr October 4, 2019 11:41

marehr reviewed Oct 5, 2019

View reviewed changes

rrahn mentioned this pull request Oct 6, 2019

Feature/add pseudo random access view #1283

Merged

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from dd089fd to e9e797f Compare October 7, 2019 16:48

rrahn requested a review from marehr October 7, 2019 16:48

rrahn added 4 commits October 7, 2019 19:08

[FEATURE] Adds pseudo random access range.

0d4681a

[MISC] Let banded trace iterator return actual matrix coordinates.

a34077c

The banded trace iterator should return the actual coordinates within the global alignment matrix and not the coordiantes of the underlying storage. This corresponding mapping is only known by the banded trace matrix.

[FEATURE] Add is_instantiable_with unary type trait.

9f9cda4

[FIX] Fixes generic assign_unaligned for iterator/sentinel paired ran…

321b6be

…ges.

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from e9e797f to 9599551 Compare October 7, 2019 17:08

marehr approved these changes Oct 8, 2019

View reviewed changes

rrahn added 2 commits October 8, 2019 16:22

[MISC] Some internal refactorings of the aligned sequence builder.

9aeec4f

[FIX] Missing range concept name update.

5205342

rrahn force-pushed the misc/refactor_aligned_sequence_builder branch from 9599551 to 5205342 Compare October 8, 2019 14:22

rrahn merged commit f11bd64 into seqan:master Oct 8, 2019

marehr mentioned this pull request Oct 10, 2019

[FIX] remove slipped in files #1290

Merged

rrahn deleted the misc/refactor_aligned_sequence_builder branch October 11, 2019 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc/refactor aligned sequence builder #1237

Misc/refactor aligned sequence builder #1237

rrahn commented Sep 3, 2019 •

edited

Loading

codecov bot commented Sep 3, 2019 •

edited

Loading

eseiler left a comment

rrahn commented Sep 12, 2019

smehringer left a comment

smehringer commented Sep 17, 2019

rrahn commented Sep 17, 2019

marehr left a comment

marehr commented Sep 18, 2019

rrahn commented Oct 4, 2019 •

edited

Loading

marehr commented Oct 4, 2019

marehr left a comment •

edited

Loading

marehr Oct 4, 2019

rrahn Oct 7, 2019

marehr Oct 5, 2019

rrahn Oct 7, 2019

marehr Oct 5, 2019

marehr Oct 5, 2019

marehr Oct 5, 2019

marehr Oct 5, 2019

rrahn Oct 7, 2019

marehr left a comment

marehr Oct 8, 2019

marehr Oct 8, 2019

marehr commented Oct 9, 2019

Misc/refactor aligned sequence builder #1237

Misc/refactor aligned sequence builder #1237

Conversation

rrahn commented Sep 3, 2019 • edited Loading

Blocked by #1283

codecov bot commented Sep 3, 2019 • edited Loading

Codecov Report

eseiler left a comment

Choose a reason for hiding this comment

rrahn commented Sep 12, 2019

smehringer left a comment

Choose a reason for hiding this comment

smehringer commented Sep 17, 2019

rrahn commented Sep 17, 2019

marehr left a comment

Choose a reason for hiding this comment

marehr commented Sep 18, 2019

rrahn commented Oct 4, 2019 • edited Loading

marehr commented Oct 4, 2019

marehr left a comment • 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

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

marehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marehr commented Oct 9, 2019

rrahn commented Sep 3, 2019 •

edited

Loading

codecov bot commented Sep 3, 2019 •

edited

Loading

rrahn commented Oct 4, 2019 •

edited

Loading

marehr left a comment •

edited

Loading