[FEATURE] seqan3::align_multiple for MSA dependent on seqan2 #1990

smehringer · 2020-07-20T12:52:13Z

Note: Changing the result type as discussed into a seqan3::multiple_sequence_alignment_result type that models a range over aligned sequences will follow in a separate PR.

codecov · 2020-09-01T06:39:45Z

Codecov Report

Merging #1990 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1990      +/-   ##
==========================================
+ Coverage   97.86%   97.88%   +0.01%     
==========================================
  Files         267      269       +2     
  Lines       10055    10145      +90     
==========================================
+ Hits         9840     9930      +90     
  Misses        215      215

Impacted Files	Coverage Δ
...clude/seqan3/alignment/multiple/align_multiple.hpp	`100.00% <100.00%> (ø)`
...ltiple/detail/align_multiple_seqan2_adaptation.hpp	`100.00% <100.00%> (ø)`
include/seqan3/io/stream/iterator.hpp	`98.52% <0.00%> (-0.03%)`	⬇️
include/seqan3/search/search.hpp	`100.00% <0.00%> (ø)`
include/seqan3/range/views/detail.hpp	`100.00% <0.00%> (ø)`
include/seqan3/search/search_result.hpp	`100.00% <0.00%> (ø)`
include/seqan3/search/fm_index/fm_index_cursor.hpp	`100.00% <0.00%> (ø)`
...lude/seqan3/search/fm_index/bi_fm_index_cursor.hpp	`100.00% <0.00%> (ø)`
...de/seqan3/argument_parser/detail/version_check.hpp	`92.74% <0.00%> (ø)`
...qan3/alignment/pairwise/alignment_configurator.hpp	`100.00% <0.00%> (ø)`
... and 5 more

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 0a02af9...f7e06a8. Read the comment docs.

eseiler

Can you go through the documentation and seqan2->SeqAn2, seqan3->SeqAn3, seqan->SeqAn, where not used as part of a type.

I'm also pretty sure, that we don't want to explicitly say SeqAn3 because of semantic versioning, but I don't see how to omit the 3 and not get entirely confused :)

Also: msa->MSA

include/seqan3/alignment/multiple/align_multiple.hpp

eseiler · 2020-09-01T09:30:08Z

include/seqan3/alignment/multiple/align_multiple.hpp

+auto align_multiple(std::vector<range_t> const & input, config_t config = align_cfg::msa_default_configuration)
+{
+#if !SEQAN3_HAS_SEQAN2 // multiple sequence alignment is only enabled with seqan2
+    static_assert(false, "You need to have seqan 2.x");


Suggested change

static_assert(false, "You need to have seqan 2.x");

static_assert(false, "You need to have SeqAn >= 2.4");

Actually we even require the develop branch, so you could even put the version of the next release (The-SeqAn3-Compatibility-Release) here....

When do we release that? :D

Shortly before 3.1 I guess

No, the next SeqAn 2 release should come shortly after 3.0.2 :)

include/seqan3/alignment/multiple/detail/align_multiple_seqan2_adaptation.hpp

test/performance/alignment/multiple_alignment_benchmark.cpp

test/unit/alignment/multiple/align_multiple_seqan2_adaptation_test.cpp

test/unit/alignment/multiple/align_multiple_test.cpp

include/seqan3/alignment/multiple/detail/align_multiple_seqan2_adaptation.hpp

smehringer · 2020-09-02T09:18:03Z

Can you go through the documentation and seqan2->SeqAn2, seqan3->SeqAn3, seqan->SeqAn, where not used as part of a type.

Done. (also not if used as a variable name etc. just docs)

I'm also pretty sure, that we don't want to explicitly say SeqAn3 because of semantic versioning, but I don't see how to omit the 3 and not get entirely confused :)

Well, in the public API I did not mention either SeqAn3 nor SeqAn2 but just in the detail documentation that actually uses both libraries. I think removing that would be too confusing and since it is hidden from the user I hope it is fine?

Also: msa->MSA

Done. (I think it was only once?)

include/seqan3/alignment/multiple/align_multiple.hpp

include/seqan3/core/platform.hpp

Co-authored-by: Simon Sasse <simonsasse97@gmail.com> Co-authored-by: Jörg Winkler <j.winkler@fu-berlin.de>

rrahn

Here are my first thoughts on the thing.
This really really great and a very good starting point.
Some things that are just in my mind for follow up things:

Make the return type an actual alignment_result and store the alignment etc. inside of it.
We might be able to build the alignment graph over origanal sequences, since the gaps are merely stored as position offsets. Then we could avoid copying everything again.
Make the input generic for ranges.

rrahn · 2020-09-17T07:09:17Z

include/seqan3/alignment/multiple/align_multiple.hpp

+#if SEQAN3_HAS_SEQAN2
+#include <seqan/graph_msa.h>
+#endif


can we put this below the seqan3 includes? Then they are not spread

include/seqan3/alignment/multiple/align_multiple.hpp

rrahn · 2020-09-17T07:12:34Z

include/seqan3/alignment/multiple/align_multiple.hpp

+ * Computes a multiple sequence alignment from the given input sequences, using a consistency-based progressive
+ * alignment algorithm on a graph of sequence segments. You can use the configuration object to
+ * specify various parameters, like gap scores, alignment scores and band constraints. The return type is
+ * `std::vector<std::vector<gapped_alphabet_type>>`, with the inner type derived from the input sequence type.


can you please also add the paper reference here? Because no one knows what consistency-based progressive alignment on graph sequence segments is supposed to mean.

rrahn · 2020-09-17T07:14:18Z

include/seqan3/alignment/multiple/align_multiple.hpp

+ * `std::vector<std::vector<gapped_alphabet_type>>`, with the inner type derived from the input sequence type.
+ */
+template <std::ranges::forward_range range_t, typename config_t = decltype(align_cfg::msa_default_configuration)>
+auto align_multiple(std::vector<range_t> const & input, config_t config = align_cfg::msa_default_configuration)


Suggested change

auto align_multiple(std::vector<range_t> const & input, config_t config = align_cfg::msa_default_configuration)

auto align_multiple(std::vector<range_t> const & input, config_t config = align_cfg::msa_default_configuration)

Was this intended, to only allow std::vector here? So I can't have a view over a collection of sequences.
It must not be changed now. It is ok to at least track this in a backlog item for the future.

yes it should definitely change in the future. I just didn't want to make the first PR to big with the additional interfaces :)

include/seqan3/alignment/multiple/detail/align_multiple_seqan2_adaptation.hpp

rrahn · 2020-09-17T08:16:52Z

include/seqan3/alignment/multiple/detail/align_multiple_seqan2_adaptation.hpp

+    template <typename candidate_t>
+    struct is_invalid_msa_config :
+        std::negation<std::disjunction<std::is_same<candidate_t, seqan3::align_cfg::band_fixed_size>,
+                                       is_type_specialisation_of<candidate_t, seqan3::align_cfg::gap>,
+                                       is_type_specialisation_of<candidate_t, seqan3::align_cfg::scoring_scheme>>>{};
+
+    /*!\brief Validate the given MSA configuration.
+     * \tparam configs_t The specified configuration elements.
+     */
+    template <typename ... configs_t>
+    void validate_configuration(seqan3::configuration<configs_t...> const &)
+    {
+        static_assert(seqan3::pack_traits::find_if<is_invalid_msa_config, configs_t...> == -1,
+                      "The given MSA configuration is not valid.");
+    }


Suggested change

template <typename candidate_t>

struct is_invalid_msa_config :

std::negation<std::disjunction<std::is_same<candidate_t, seqan3::align_cfg::band_fixed_size>,

is_type_specialisation_of<candidate_t, seqan3::align_cfg::gap>,

is_type_specialisation_of<candidate_t, seqan3::align_cfg::scoring_scheme>>>{};

/*!\brief Validate the given MSA configuration.

* \tparam configs_t The specified configuration elements.

*/

template <typename ... configs_t>

void validate_configuration(seqan3::configuration<configs_t...> const &)

{

static_assert(seqan3::pack_traits::find_if<is_invalid_msa_config, configs_t...> == -1,

"The given MSA configuration is not valid.");

}

template <typename candidate_t>

static constexpr bool is_valid_msa_config =

std::is_same_v<candidate_t, seqan3::align_cfg::band_fixed_size> ||

is_type_specialisation_of<candidate_t, seqan3::align_cfg::gap> ||

is_type_specialisation_of<candidate_t, seqan3::align_cfg::scoring_scheme>;

/*!\brief Validate the given MSA configuration.

* \tparam configs_t The specified configuration elements.

*/

template <typename ... configs_t>

void validate_configuration(seqan3::configuration<configs_t...> const &)

{

static_assert(!(is_valid_msa_config<configs_t> && ...), "The given MSA configuration is not valid.");

}

That would be solution one, which is I think less difficult to understand.

And before we do this. Is that really necessary?

And before we do this. Is that really necessary?

Well, we could also just ignore all other configuration types but I thought it might be good to know for the user which are allowed instead of just silently ignoring them 🤷

rrahn · 2020-09-17T08:57:50Z

include/seqan3/alignment/multiple/detail/align_multiple_seqan2_adaptation.hpp

+    using seqan3_types = type_list<dna4, dna5, dna15, rna4, rna5, aa27, aa10murphy, aa10li>;
+    //!\brief The corresponding list of SeqAn2 types (the order must be the same!).
+    using seqan2_types = type_list<seqan::Dna,
+                                   seqan::Dna5,
+                                   seqan::Iupac,
+                                   seqan::Rna,
+                                   seqan::Rna5,
+                                   seqan::AminoAcid,
+                                   seqan::ReducedAminoAcid<seqan::Murphy10>,
+                                   seqan::ReducedAminoAcid<seqan::Li10>>;
+    //!\brief The index of the seqan3_alphabet_type in the list of seqan3_types.
+    static constexpr auto index = list_traits::find<seqan3_alphabet_type, seqan3_types>;


I am not completely convinced, that this is the correct approach. It doesn't scale nor does it make sense, because it destroys the entire idea of being generic. I know it is a first draft and I am willing to keep this for now, to also get along with other things but since we are going to work more with SeqAn2 in the future we should generally think about the compatibility.

Brain Dump

Basically there are two ways to go along:

Add a seqan::SimpleType<char, SeqAn3AlphabetAdaptor> that reduces any alphabet we pass to its char value

Make all seqan::SimpleType interface functions available for types that model seqan3::[writable_]alphabet.

The first approach has the advantage, that it we basically only need to define this type and overload BitsPerValue and ValueSize. I think all the remaining functions would resolve to defaults for char.
The second approach has the advantage, that we could more fine grained control concepts like nucleotide_alphabet etc. But it is much more verbose and we might not need this finer control anyway.
In the optimal case, if we only use the iterator interfaces inside of the seqan algorithms, we should be able to also pass along our sequence with a transform view to the adaptor alphabet. We probably have to do some work to make the meta functions in SeqAn compatible though. But that should be in general possible. If we would then (which I have on my list of things to do anyway) also make the seqan::SimpleType model seqan3::alphabet etc. we would/could basically forward the output to the seqan3 world without transforming it before. Of course there are more bits to it and this has to be evaluated. But that would be the way to go I think.

Alright, sounds like a good plan. But it also sounds like this might take some time because we can't estimate how easy it will be to adapt the interfaces. I would take this on as a follow up that we can discuss and schedule afterwards if that is fine?

I made a PR against your repo branch a while ago. There it is already removed and done via char only. Have you seen it?

rrahn · 2020-09-17T13:13:44Z

test/unit/alignment/multiple/align_multiple_seqan2_adaptation_test.cpp

+
+    seqan::Blosum62 expected_matrix{};
+    for (size_t i = 0; i < static_cast<size_t>(seqan::Blosum62::TAB_SIZE); ++i)
+        EXPECT_EQ(msaOpt.sc.data_tab[i], expected_matrix.data_tab[i]);


well, the check should not be on the internals but on the public interfaces.

I don't know if I get you right here, why should we not test the detail interfaces?
Note that the public interface also tests the BLOSUM matrix for the correct MSA (see align_multiple_test).

rrahn · 2020-09-17T13:14:36Z

include/seqan3/alignment/multiple/align_multiple.hpp

+ * See seqan3::detail::align_multiple_seqan2_adaptation::create_msa_configuration for more details on the configuration.
+ * \endif
+ */
+constexpr configuration msa_default_configuration = gap{gap_scheme{gap_score{-1}, gap_open_score{-13}}};


I think this is not required. The adaptor already selects the defaults and we can simply use an empty seqan3::configration as the optinal argument if that is ok with the algorithm.

I thought this makes the default (that I can configure) more explicit but sure I can move this into the adaptions.

Co-authored-by: rrahn <rene.rahn@fu-berlin.de>

smehringer · 2020-10-14T09:07:43Z

* Make the return type an actual alignment_result and store the alignment etc. inside of it.

Yes, already a follow up!

* We might be able to build the alignment graph over original sequences, since the gaps are merely stored as position offsets. Then we could avoid copying everything again.

I tried to use std::vectors and seqan3 alphabets but I could several deep down errors that I couldn't understand (although I did not put much effort into it). This would be a follow up that needs to be investigated also considering you suggestions to use a simple char type as adaptation.

* Make the input generic for ranges.

Yes.

smehringer · 2022-09-16T10:13:01Z

the MSA is not used right now. Before having an actual use case we will not pull in the MSA module

smehringer mentioned this pull request Jul 20, 2020

Check if we can easily integrate seqan2 for the seqan3::align_multiple interface seqan/product_backlog#158

Closed

smehringer force-pushed the tcoffee branch from 6fcbdc9 to b18a59d Compare August 20, 2020 09:31

smehringer force-pushed the tcoffee branch from 51d0498 to 4d5ef90 Compare August 31, 2020 15:47

smehringer force-pushed the tcoffee branch from b949700 to d9b16c3 Compare September 1, 2020 07:18

smehringer changed the title ~~First draft to peek onto seqan3::align_multiple~~ [FEATURE] seqan3::align_multiple for MSA dependent on seqan2 Sep 1, 2020

smehringer marked this pull request as ready for review September 1, 2020 07:19

smehringer requested a review from eseiler September 1, 2020 07:19

eseiler requested changes Sep 1, 2020

View reviewed changes

smehringer commented Sep 2, 2020

View reviewed changes

include/seqan3/alignment/multiple/detail/align_multiple_seqan2_adaptation.hpp Outdated Show resolved Hide resolved

smehringer commented Sep 2, 2020

View reviewed changes

include/seqan3/alignment/multiple/detail/align_multiple_seqan2_adaptation.hpp Outdated Show resolved Hide resolved

smehringer requested a review from eseiler September 2, 2020 14:20

eseiler approved these changes Sep 2, 2020

View reviewed changes

include/seqan3/alignment/multiple/align_multiple.hpp Outdated Show resolved Hide resolved

include/seqan3/core/platform.hpp Outdated Show resolved Hide resolved

eseiler requested a review from rrahn September 2, 2020 15:10

smehringer force-pushed the tcoffee branch from 9d81cd1 to db3949d Compare September 3, 2020 06:19

smehringer and others added 6 commits September 17, 2020 08:09

[MISC] Move SEQAN3_HAS_SEQAN2 into platform.hpp.

a10d8b1

[FEATURE] Add seqan3::align_multiple algorithm dependent on seqan2.

dd2d42c

Co-authored-by: Simon Sasse <simonsasse97@gmail.com> Co-authored-by: Jörg Winkler <j.winkler@fu-berlin.de>

[TEST] Add tests for seqan3::align_multiple and detail code.

49826df

Co-authored-by: Simon Sasse <simonsasse97@gmail.com> Co-authored-by: Jörg Winkler <j.winkler@fu-berlin.de>

[TEST] Add benchmark for seqan3::align_multiple

b23f9e7

Co-authored-by: Simon Sasse <simonsasse97@gmail.com> Co-authored-by: Jörg Winkler <j.winkler@fu-berlin.de>

[DOC] Add Changelog entry.

6aeda23

Remove unneccessary seqan2 inlcudes.

cfbd2b9

smehringer force-pushed the tcoffee branch from db3949d to cfbd2b9 Compare September 17, 2020 06:10

rrahn requested changes Sep 17, 2020

View reviewed changes

Apply suggestions from code review

f7e06a8

Co-authored-by: rrahn <rene.rahn@fu-berlin.de>

marehr mentioned this pull request May 21, 2021

[Alignment] Global MSA with free end-gaps for a subset of sequences #2674

Closed

smehringer closed this Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] seqan3::align_multiple for MSA dependent on seqan2 #1990

[FEATURE] seqan3::align_multiple for MSA dependent on seqan2 #1990

smehringer commented Jul 20, 2020 •

edited

Loading

codecov bot commented Sep 1, 2020 •

edited

Loading

eseiler left a comment

eseiler Sep 1, 2020

smehringer Sep 2, 2020

eseiler Sep 2, 2020

rrahn Sep 17, 2020

smehringer commented Sep 2, 2020

rrahn left a comment

rrahn Sep 17, 2020

rrahn Sep 17, 2020

rrahn Sep 17, 2020

smehringer Oct 14, 2020 •

edited

Loading

rrahn Sep 17, 2020

smehringer Oct 14, 2020

rrahn Sep 17, 2020

smehringer Oct 14, 2020

rrahn Oct 14, 2020

rrahn Sep 17, 2020

smehringer Oct 14, 2020

rrahn Sep 17, 2020

smehringer Oct 14, 2020

smehringer commented Oct 14, 2020

smehringer commented Sep 16, 2022

	static_assert(false, "You need to have seqan 2.x");
	static_assert(false, "You need to have SeqAn >= 2.4");

	auto align_multiple(std::vector<range_t> const & input, config_t config = align_cfg::msa_default_configuration)
	auto align_multiple(std::vector<range_t> const & input, config_t config = align_cfg::msa_default_configuration)

[FEATURE] seqan3::align_multiple for MSA dependent on seqan2 #1990

[FEATURE] seqan3::align_multiple for MSA dependent on seqan2 #1990

Conversation

smehringer commented Jul 20, 2020 • edited Loading

codecov bot commented Sep 1, 2020 • edited Loading

Codecov Report

eseiler left a 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

smehringer commented Sep 2, 2020

rrahn left a 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 Oct 14, 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

Brain Dump

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

smehringer commented Oct 14, 2020

smehringer commented Sep 16, 2022

smehringer commented Jul 20, 2020 •

edited

Loading

codecov bot commented Sep 1, 2020 •

edited

Loading

smehringer Oct 14, 2020 •

edited

Loading