Add semialphabet any #981

sarahet · 2019-05-16T06:41:59Z

No description provided.

codecov · 2019-05-16T08:22:23Z

Codecov Report

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

@@            Coverage Diff             @@
##           master     #981      +/-   ##
==========================================
+ Coverage   96.38%   96.41%   +0.02%     
==========================================
  Files         181      181              
  Lines        6635     6633       -2     
==========================================
  Hits         6395     6395              
+ Misses        240      238       -2

Impacted Files	Coverage Δ
...ude/seqan3/alphabet/composite/semialphabet_any.hpp	`100% <100%> (ø)`
include/seqan3/core/bit_manipulation.hpp	`92.3% <0%> (-1.03%)`	⬇️
...qan3/alignment/pairwise/edit_distance_unbanded.hpp	`82.97% <0%> (-0.81%)`	⬇️
include/seqan3/io/stream/debug_stream.hpp	`94.44% <0%> (-0.48%)`	⬇️
.../pairwise/execution/alignment_executor_two_way.hpp	`97.05% <0%> (-0.09%)`	⬇️
...e/seqan3/alignment/matrix/alignment_coordinate.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/io/alignment_file/input.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/alphabet/nucleotide/dna5.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/alphabet/aminoacid/aa10li.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/alphabet/nucleotide/rna4.hpp	`100% <0%> (ø)`	⬆️
... and 30 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 495f648...852e4af. Read the comment docs.

h-2

This looks pretty good! Mostly remarks regarding the example!

h-2 · 2019-05-16T08:45:37Z

include/seqan3/alphabet/composite/semialphabet_any.hpp

+ * of the same size.
+ * It is therefore possible to convert the semialphabet_any into an alphabet type that is not the original
+ * alphabet type. However, this should either be avoided or used with care as no warnings are given when attempting
+ * to convert the semialphabet_any into a type that is not comparable to the original alphabet type.


Good description 👍

I would maybe add one more sentence:

The main advantage of using this data structure is to reduce instantiation of templates when using multiple alphabets of the same size and either their character representation is not important or they are reified at a later point in the program.

h-2 · 2019-05-16T09:07:37Z

include/seqan3/alphabet/composite/semialphabet_any.hpp

+ * It is therefore possible to convert the semialphabet_any into an alphabet type that is not the original
+ * alphabet type. However, this should either be avoided or used with care as no warnings are given when attempting
+ * to convert the semialphabet_any into a type that is not comparable to the original alphabet type.
+ *


Add before the snippet

### Example

h-2 · 2019-05-16T09:08:17Z

include/seqan3/alphabet/composite/semialphabet_any.hpp

+ * to convert the semialphabet_any into a type that is not comparable to the original alphabet type.
+ *
+ * \snippet test/snippet/alphabet/composite/semialphabet_any.cpp example
+ */


Maybe add:

\see https://en.wikipedia.org/wiki/Type_erasure \see https://en.wikipedia.org/wiki/Reification_(computer_science)

h-2 · 2019-05-16T09:10:12Z

include/seqan3/alphabet/composite/semialphabet_any.hpp

+
+    //!\brief Construct semialphabet_any from alphabet of the same size
+    template <Semialphabet other_alph_t>
+        requires alphabet_size_v<other_alph_t> == size


Suggested change

requires alphabet_size_v<other_alph_t> == size

//!\cond

requires (alphabet_size_v<other_alph_t> == size)

//!\endcond

We need to hide the requires clause from doxygen and we need to also put () around most terms now (because clang requires this).

Also for the conversion operator

h-2 · 2019-05-16T09:15:30Z

include/seqan3/alphabet/composite/semialphabet_any.hpp

+    //!\brief Construct semialphabet_any from alphabet of the same size
+    template <Semialphabet other_alph_t>
+        requires alphabet_size_v<other_alph_t> == size
+    semialphabet_any(other_alph_t const other)


Please make both the constructor and the conversion operator explicit. This will at least prevent the conversions from happening automatically and force people to use static_cast (or direct initialisation).

h-2 · 2019-05-16T09:16:33Z

include/seqan3/alphabet/composite/semialphabet_any.hpp

+        requires alphabet_size_v<other_alph_t> == size
+    semialphabet_any(other_alph_t const other)
+    {
+        using seqan3::to_rank;


this weird stuff is not required anymore since #943

Not required because it needs to be replaced by something else? Or not required and should just work without?

Because I get an error when I remove it and this PR is so huge I can not immediately find an answer 😄

Write seqan3:: before the calls in the line below instead. This is only necessary if you have member functions of the same name. Otherwise you can omit the seqan3:: entirely.

Write seqan3:: before the calls in the line below instead.

Just before to_rank()

h-2 · 2019-05-16T09:17:07Z

include/seqan3/alphabet/composite/semialphabet_any.hpp

+    }
+};
+
+}


Suggested change

}

} // namespace seqan3

💅

h-2 · 2019-05-16T09:47:19Z

test/snippet/alphabet/composite/semialphabet_any.cpp

+    // This is possible however should be used with care since there will be no warning given when attempting to convert
+    // a semialphabet_any into an alphabet that is not the original alphabet. This may lead to errors when transforming
+    // the semialphabet_any into an alphabet type that is not comparable to the original type.
+    aa10murphy_vector v4 = v2;


Hm, but this is what we want people not to do!

I would maybe make the example like this (roughly sketched and not tested, bad grammar)

// print is a template and gets instantiated two times, // because the behaviour is different for both types template <typename rng_t> void print(rng_t && r) { seqan3::debug_stream << r << '\n'; } // algorithm is not a template, only one instance is generated by the compiler; // type information is encoded via a run-time parameter void algorithm(std::vector<semialphabet_any<10>> & r, bool is_murphy) { // a silly algorithm example that replaces rank 0 with rank 1: for (auto & v : r) if (seqan3::to_rank(v) == 0) seqan3::assign_rank_to(1, v); // here we reify the type for printing if (is_murphy) print(v | seqan3::view::convert<aa10murphy>); else print(v | seqan3::view::convert<aa10li>); } // two instances of algo_pre exist; they type erase the different arguments to the same type // and encode the type information as a run-time parameter void algo_pre(aa10li_vector const & v) { seqan3::semialphabet_any<10> tmp = v | seqan3::view::convert<semialphabet_any<10>>; algo(tmp, false); } void algo_pre(aa10murphy_vector const & v) { seqan3::semialphabet_any<10> tmp = v | seqan3::view::convert<semialphabet_any<10>>; algo(tmp, true); } int main() { seqan33::aa10li_vector v1 {...}; algo_pre(v1); seqan3::aa10murphy_vector v2 {...}; algo_pre(v2); }

And explanation:

The example illustrates how we can reduce the usage of templates (or the amount of different instantiations) via type erasure. Having only one function generated for `algo()` is the only benefit of using `semialphabet_any` here. Of course this only makes sense for your application if the part of the program that is agnostic of the character representation (your equivalent of `algo()`) is substantially larger than the specific parts – and if compile-time and/or size of the exectuble are a concern.

BTW: we don't use using namespace seqan3; in the snippets anymore...

h-2 · 2019-05-16T09:50:05Z

include/seqan3/alphabet/composite/semialphabet_any.hpp

+ * \ingroup composite
+ * \implements seqan3::TriviallyCopyable
+ * \implements seqan3::StandardLayout
+ *


\implements seqan3::Semialphabet

h-2 · 2019-05-17T17:34:01Z

include/seqan3/alphabet/composite/semialphabet_any.hpp

+    //!\brief Enable conversion of semialphabet_any into other (semi-)alphabet of the same size
+    template <Semialphabet other_alph_t>
+        requires alphabet_size_v<other_alph_t> == size
+    operator other_alph_t()


your conversion operator needs to be const-qualified, otherwise you imply that the function will change the object and it won't be possible to call it in a const-context, like in view::convert.

sarahet requested a review from h-2 May 16, 2019 06:41

sarahet assigned h-2 May 16, 2019

sarahet force-pushed the add_semialphabet_any branch from 78d4dbd to 8a93925 Compare May 16, 2019 08:22

sarahet force-pushed the add_semialphabet_any branch from 8a93925 to bda43fe Compare May 16, 2019 08:27

h-2 reviewed May 16, 2019

View reviewed changes

h-2 reviewed May 17, 2019

View reviewed changes

sarahet added 2 commits May 21, 2019 11:16

[feature] Add semialphabet_any

1736ad5

[fix] change cpp reference link to new address

852e4af

sarahet force-pushed the add_semialphabet_any branch from bda43fe to 852e4af Compare May 21, 2019 09:17

h-2 approved these changes May 21, 2019

View reviewed changes

h-2 merged commit 8c66b64 into seqan:master May 21, 2019

sarahet mentioned this pull request May 24, 2019

semialphabet_any #878

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add semialphabet any #981

Add semialphabet any #981

sarahet commented May 16, 2019

codecov bot commented May 16, 2019 •

edited

Loading

h-2 left a comment

h-2 May 16, 2019

h-2 May 16, 2019

h-2 May 16, 2019

h-2 May 16, 2019

h-2 May 16, 2019

h-2 May 16, 2019

h-2 May 16, 2019

h-2 May 16, 2019

sarahet May 17, 2019

h-2 May 17, 2019

h-2 May 17, 2019

h-2 May 16, 2019

h-2 May 16, 2019

h-2 May 16, 2019

h-2 May 16, 2019

h-2 May 17, 2019

-        requires alphabet_size_v<other_alph_t> == size
+//!\cond
+        requires (alphabet_size_v<other_alph_t> == size)
+//!\endcond

+                  }
+              };
+              }

Add semialphabet any #981

Add semialphabet any #981

Conversation

sarahet commented May 16, 2019

codecov bot commented May 16, 2019 • edited Loading

Codecov Report

h-2 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 16, 2019 •

edited

Loading