alphabet_base expects functions instead of look-up tables. #2427
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/seqan/seqan3/FVGwq8hgw9v9qLeKd7pTPTYdQpJs |
marehr
requested review from
a team and
simonsasse
and removed request for
a team
marehr
force-pushed
the
alphabet_base_functions
branch
from
afbbc26 to
2537f10
Compare
Codecov Report
@@ Coverage Diff @@
## master #2427 +/- ##
==========================================
+ Coverage 98.29% 98.31% +0.01%
==========================================
Files 268 269 +1
Lines 11055 11134 +79
==========================================
+ Hits 10867 10946 +79
Misses 188 188
Continue to review full report at Codecov.
|
marehr
force-pushed
the
alphabet_base_functions
branch
2 times, most recently
from
de9c83e to
1cd0f8e
Compare
marehr
force-pushed
the
alphabet_base_functions
branch
from
2aafb9d to
d270fe0
Compare
@simonsasse, can you approve and assign core for review? |
There was a problem hiding this comment.
Looks good in general, just some documentation remarks and general questions.
This point does not need / should not be addressed in this PR:
I am missing a bit some documentation on what the advantage of this is. If there is a changelog entry, it should shortly describe why this is good?
I think doc/howto/write_an_alphabet/dna2_derive_from_base.cpp can also shortly mention that there is an alternative to tables (it does show it, but doesn't explicitly mention it).
| using index_t = std::make_unsigned_t<char_type>; | ||
| index_t const id = static_cast<index_t>(chr); | ||
| return derived_type::char_to_rank[id]; |
There was a problem hiding this comment.
Why are these lines different?
From what I've read, the bug only refers to deprecation warnings being emitted even when the code is not called.
So this block should also be
using index_t = std::make_unsigned_t<char_type>;
return derived_type::char_to_rank[static_cast<index_t>(chr)];?
include/seqan3/alphabet/gap/gap.hpp
Outdated
| @@ -43,10 +43,16 @@ class gap : public alphabet_base<gap, 1, char> | |||
| friend base_t; | |||
|
|
|||
| //!\brief Value to char conversion table. | |||
marehr
force-pushed
the
alphabet_base_functions
branch
2 times, most recently
from
91f8071 to
7b001db
Compare
|
I added / changed documentation, I tried to clarify the snippets. Otherwise, I used your suggestion not to use the term lookup table, but just a general description. |
I tried to define the lookup tables within the function itself, but we
can't declare the lookup table as `static constexpr ...`, because the
function is declared constexpr and does not allow any linkage within it.
So we would need to declare the lookup table as constexpr only, like so
```patch
- static constexpr char_type rank_to_char[alphabet_size]
+ static constexpr char_type rank_to_char(rank_type const rank)
+ {
+ constexpr char_type lookup_table[] =
{
'A',
'C',
'G',
'T'
};
+ return lookup_table[rank];
+ }
```
The problem is that gcc does not optimise (clang does!) this case with
lookup tables, but moves the memory-in every time we call this function.
See https://godbolt.org/z/K8fcqe.
The micro benchmark `test/performance/alphabet/alphabet_assign_char_benchmark`
showed that `assign_char<seqan3::dna4>` took `81.6 ns` before that
patch, but `2903 ns` after that change.
So I reverted to use the normal in-class lookup-table declaration that is now
used within the static function itself.
I would have preferred to remove that class-visible definition.
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99320
marehr
force-pushed
the
alphabet_base_functions
branch
from
7b001db to
a9ba4d7
Compare
This is part of seqan/product_backlog#295
This changes
rank_to_charandchar_to_rankto static member functions.Blocked by #2430.