-
Notifications
You must be signed in to change notification settings - Fork 83
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] Allow to pass pairs to pairwise alignment #1913
[FEATURE] Allow to pass pairs to pairwise alignment #1913
Conversation
a1d5603
to
1b4f438
Compare
Codecov Report
@@ Coverage Diff @@
## master #1913 +/- ##
=======================================
Coverage 97.77% 97.77%
=======================================
Files 258 258
Lines 9687 9688 +1
=======================================
+ Hits 9471 9472 +1
Misses 216 216
Continue to review full report at Codecov.
|
a15e248
to
bac80f3
Compare
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!
bac80f3
to
b886649
Compare
//!\cond | ||
requires tuple_like<sequence_t> && !detail::align_pairwise_single_input<std::remove_reference_t<sequence_t>> | ||
//!\endcond | ||
constexpr auto align_pairwise(sequence_t && seq, alignment_config_t const & config) |
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.
Ok, I thought about it, and this is how I'd like to tackle this.
Replace this definition:
SEQAN3_CONCEPT align_pairwise_single_input = |
with the following code:
SEQAN3_CONCEPT align_pairwise_single_input =
sequence_pair<std::remove_reference_t<t>> &&
std::is_lvalue_reference_v<t> ||
(std::ranges::viewable_range<std::tuple_element_t<0, std::remove_reference_t<t>>> &&
std::ranges::viewable_range<std::tuple_element_t<1, std::remove_reference_t<t>>>);
Then replace this line:
requires detail::align_pairwise_single_input<std::remove_reference_t<sequence_t>> && |
with this:
requires detail::align_pairwise_single_input<sequence_t> &&
And then you can rewrite the body above as:
using std::get;
if constexpr (std::is_lvalue_reference_v<sequence_t>) // Forward tuple elements as references.
return align_pairwise(std::tie(get<0>(seq), get<1>(seq)), config);
static_assert(std::tuple_size_v<std::remove_reference_t<sequence_t>> == 2,
"Alignment configuration error: Expects exactly two sequences for pairwise alignments.");
static_assert(std::ranges::viewable_range<std::tuple_element_t<0, std::remove_reference_t<sequence_t>>> &&
std::ranges::viewable_range<std::tuple_element_t<1, std::remove_reference_t<sequence_t>>>,
"Alignment configuration error: The tuple elements must model std::ranges::viewable_range.");
return align_pairwise(std::views::single(std::forward<sequence_t>(seq)), config);
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.
Then we don't need the this additional interface.
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.
Many thanks for tackling this 👍 . I would approach the thing a little differently, though 😁 .
} | ||
|
||
// use make_pair | ||
|
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.
} | ||
|
||
// use std::pair | ||
|
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.
@@ -58,15 +58,73 @@ auto call_alignment(seq_t && seq, cfg_t && cfg) | |||
|
|||
TYPED_TEST(align_pairwise_test, single_pair) | |||
{ | |||
// use std::tie | |||
|
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.
f8edcc8
to
b5c98a9
Compare
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 so far. Please rebase onto current master to resolve the merge conflict.
b5c98a9
to
2b8a8c8
Compare
(cherry picked from commit db9a853)
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
2b8a8c8
to
6ba8815
Compare
Resolves #1516