second phase of alphabet restructuring #943

h-2 · 2019-04-30T14:32:34Z

I have switched to a local fork to workaround USCiLab/cereal#565 for now.

Changes the alphabet related functions to be function objects. See also #869

I recommend reviewing on a commit-by-commit basis.

marehr · 2019-04-30T14:46:54Z

🚧 WIP 🚧 👷‍♂️ 👷‍♀️ 🏗️ 😆

h-2 · 2019-05-03T09:25:33Z

@eseiler Can you review this? Thanks!

include/seqan3/core/char_operations/pretty_print.hpp

include/seqan3/alphabet/concept.hpp

test/unit/alphabet/custom_alphabet_test.cpp

smehringer

first tiny look

include/seqan3/io/stream/parse_condition.hpp

include/seqan3/core/char_operations/pretty_print.hpp

include/seqan3/alphabet/concept.hpp

h-2 · 2019-05-03T19:10:43Z

I have added long(er) snippets to everything and addressed most of your points I think. I have also restructured the documentation for the customisation points a little so that it is hopefully more helpful. I have also added a second custom alphabet test that tests the "not-default-constructible" overloads (the nice adl-hack through associated namespaces of template args).

Also requested @rrahn for review.

If cereal does not react until Monday I will fork the repo and we need to discuss what to do before the release.

include/seqan3/alphabet/concept.hpp

test/performance/alphabet/alphabet_benchmark.cpp

codecov · 2019-05-07T15:20:25Z

Codecov Report

Merging #943 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #943      +/-   ##
==========================================
- Coverage   96.66%   96.64%   -0.02%     
==========================================
  Files         178      179       +1     
  Lines        6557     6528      -29     
==========================================
- Hits         6338     6309      -29     
  Misses        219      219

Impacted Files	Coverage Δ
include/seqan3/io/stream/parse_condition.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/io/stream/debug_stream.hpp	`94.44% <ø> (ø)`	⬆️
include/seqan3/alphabet/exception.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/alphabet/detail/member_exposure.hpp	`100% <ø> (ø)`	⬆️
...nclude/seqan3/io/stream/parse_condition_detail.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/alphabet/detail/alphabet_base.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/alphabet/detail/alphabet_proxy.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/alphabet/concept.hpp	`100% <100%> (ø)`	⬆️
...clude/seqan3/core/char_operations/pretty_print.hpp	`100% <100%> (ø)`
include/seqan3/alphabet/cigar/cigar_op.hpp	`100% <100%> (ø)`	⬆️
... and 8 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 c910631...281b8fb. Read the comment docs.

h-2 · 2019-05-07T15:25:18Z

I have DRYed a lot of the CPO-definitions so the number of added lines is reduces by about 400; there are no more "auxiliary concepts" and also no more free functions inside detail::adl::only. This required adding a small macro, though. Return types are now properly enforced and I fixed some smaller bugs, too.

Note that the CPOs are not in anonymous namespaces, because doxygen handles this really badly. It's not necessary though, unless we want to add friends to our own classes within namespace seqan3 that have the same names as the CPOs [we can just use members for this and friends of external types are handled correctly].

please give this a review soon everyone, thanks!

h-2 · 2019-05-07T15:28:41Z

The design in this file is based loosely on:

(the anonymous namespace and static hackery in the proposal are not necessary, because we have inline variables now)

eseiler

First look

include/seqan3/core/detail/customisation_point.hpp

include/seqan3/core/metafunction/basic.hpp

include/seqan3/alphabet/adaptation/char.hpp

include/seqan3/alphabet/adaptation/uint.hpp

include/seqan3/alphabet/concept.hpp

rrahn

Some general questions to the code.

include/seqan3/alphabet/all.hpp

include/seqan3/alphabet/concept.hpp

rrahn · 2019-05-08T09:50:09Z

include/seqan3/io/stream/parse_condition_detail.hpp

@@ -101,7 +102,7 @@ class parse_condition_base;
 *
 * \details
 *
- * The must be invocable with an seqan3::char_adaptation_concept type and supply a static constexpr `msg` member of type
+ * The must be invocable with an std::Integral type and supply a static constexpr `msg` member of type


not resolved?

rrahn · 2019-05-08T09:52:03Z

test/snippet/alphabet/assign_rank_to.cpp

+    seqan3::dna5 d2 = seqan3::assign_rank_to(2, seqan3::dna5{});
+
+    // too-large ranks are undefined behaviour:
+    seqan3::dna5 d3 = seqan3::assign_rank_to(50, seqan3::dna5{}); // 💣💥


Please do not use 💣 💥 in comments.

Why not? 🤔

hm, ok 😿

rrahn · 2019-05-08T09:53:36Z

test/unit/alphabet/custom_alphabet2_test.cpp

+    }
+}
+
+}


// my_namespace

not resolved?

rrahn · 2019-05-08T09:54:01Z

test/unit/alphabet/custom_alphabet_test.cpp

+    }
+}
+
+}


// my_namespace

not resolved?

h-2 · 2019-05-08T14:59:39Z

But it is not resolved.

I can't reply to that inline :| I don't understand what you mean. Because I say "alphabet object"? I have already removed the requirement for the concept... What else would you call it? This is as "correct" as is understandable for the general use-case I think.

The only 100% formally correct description would be "Assign an object of type alphabet_rank_t<your_type> to your type where your_type is a type that provides one of three overloads described below".
That would make very poor UX I think. 99.99% of the time people will look at this documentation item they want to call assign_rankt_to on an existing alphabet.

h-2 · 2019-05-08T15:36:45Z

I have updated the PR. There is some weird failure in an entirely unrelated alignment snippet, I really don't know what's going on. Apparantly it's trying to invoke to_char() with an operator& defined on ios
🤷‍♂️

any ideas someone?

h-2 · 2019-05-09T11:14:03Z

Another question that we can also discuss later is whether we want a general customisation namespace, e.g. seqan3::customisation which we use for customisation point implementations for types outside of seqan (built-in, std:: maybe) and as "upload" namespace ¹ in which people can provide specialisations for types outside of their namespaces.

This would mean renaming seqan3::adaptation to seqan3::customisation or seqan3::custom. I am in favour of this, but we can easily perform this change later, it will just change a few lines here.

¹ https://quuxplusone.github.io/blog/2018/03/19/customization-points-for-functions/

rrahn

I have some more small issues. I resolved all comments that have been addressed. Some were not.

include/seqan3/alphabet/concept.hpp

include/seqan3/alphabet/detail/alphabet_proxy.hpp

rrahn · 2019-05-09T11:53:54Z

include/seqan3/core/detail/customisation_point.hpp

+                                     t && v,                                                                         \
+                                     [[maybe_unused]] arg_ts && ... args)                                            \
+    noexcept(noexcept(TERM))                                                                                         \
+    requires requires (seqan3::detail::priority_tag<PRIO> const &, t && v, [[maybe_unused]] arg_ts && ... args)      \


I don't understand why you need seqan3::detail::priority_tag<PRIO> const & in the requires clause.

Wow, I have just reverted removing this, because it's the only way doxygen will accept the above code. Note that due to the macro I cannot put //!\cond blocks around the requires and /*\cond*/ does not work. Apparently the :: confuses Doxygen in that line so it stops parsing the rest of the line which makes everything work. If I remove the tag it somehow thinks the arguments to requires requires are variable definitions of type requires 🤷‍♂️

In any case I have reverted this and documented the hack.

rrahn · 2019-05-09T12:17:44Z

include/seqan3/core/metafunction/basic.hpp

+ * \tparam dependent_ts Any provided types are ignored.
+ */
+template <typename t, typename ...dependent_ts>
+struct deferred_type


I don't think that this should be public API. The use case is very special and would require at least a snippet to get the idea. Instead I would put it into detail and maybe even in the adl stuff/customisation point stuff.

I can think of other places where this could be useful, but I can also move it to detail, if you prefer.

rrahn · 2019-05-09T12:19:16Z

test/snippet/alphabet/assign_rank_to.cpp

+    seqan3::dna5 d2 = seqan3::assign_rank_to(2, seqan3::dna5{});
+
+    // too-large ranks are undefined behaviour:
+//    seqan3::dna5 d3 = seqan3::assign_rank_to(50, seqan3::dna5{}); // 💣💥


Can you please remove 💣 💥 from the comment?

h-2 · 2019-05-09T17:13:31Z

There is some weird failure in an entirely unrelated alignment snippet, I really don't know what's going on.

@rrahn did you have a look at this? Do you have a clue whats going on?

h-2 · 2019-05-10T00:07:11Z

@rrahn did you have a look at this? Do you have a clue whats going on?

I have a workaround that can hopefully be reverted once the alphabet concepts are split with the next PR.

h-2 · 2019-05-10T14:36:04Z

I have a workaround that can hopefully be reverted once the alphabet concepts are split with the next PR.

I have found a more permanent fix to the problem that makes the workaround obsolete.

rrahn

lgtm

outdated

h-2 force-pushed the alph_refac branch from 54bbf0c to 3dedd86 Compare May 2, 2019 16:59

h-2 requested review from smehringer and eseiler and removed request for smehringer May 2, 2019 17:01

h-2 force-pushed the alph_refac branch from 3dedd86 to a7539b8 Compare May 3, 2019 09:48

eseiler requested changes May 3, 2019

View reviewed changes

smehringer reviewed May 3, 2019

View reviewed changes

h-2 force-pushed the alph_refac branch from a7539b8 to 1883170 Compare May 3, 2019 19:08

h-2 requested a review from rrahn May 3, 2019 19:11

smehringer previously requested changes May 6, 2019

View reviewed changes

include/seqan3/alphabet/concept.hpp Outdated Show resolved Hide resolved

test/performance/alphabet/alphabet_benchmark.cpp Show resolved Hide resolved

[FIX] check integral instead of char

ccd31b7

h-2 force-pushed the alph_refac branch 2 times, most recently from 8576f1a to 32e2fb9 Compare May 6, 2019 10:17

h-2 requested review from smehringer and eseiler and removed request for smehringer May 6, 2019 12:20

h-2 force-pushed the alph_refac branch from 32e2fb9 to 8821ac6 Compare May 7, 2019 15:20

eseiler requested changes May 7, 2019

View reviewed changes

rrahn requested changes May 8, 2019

View reviewed changes

[INFRA] move pretty printing of chars from stream to core

16dc5ac

h-2 force-pushed the alph_refac branch from 8821ac6 to 186cd6a Compare May 8, 2019 15:33

h-2 requested a review from eseiler May 8, 2019 15:33

h-2 requested review from rrahn and eseiler and removed request for eseiler May 8, 2019 15:33

rrahn requested changes May 9, 2019

View reviewed changes

h-2 force-pushed the alph_refac branch from 186cd6a to cc36f6b Compare May 10, 2019 00:05

h-2 requested a review from rrahn May 10, 2019 00:06

eseiler approved these changes May 10, 2019

View reviewed changes

h-2 added 2 commits May 10, 2019 11:20

[FEATURE] some helpers for customisation point definitions

f3f6e54

[FEATURE] two new metafunctions

72f358a

h-2 force-pushed the alph_refac branch from cc36f6b to 281b8fb Compare May 10, 2019 09:20

h-2 added 5 commits May 10, 2019 16:32

[INFRA] change to CPO for alphabet concepts

dd65b3f

[INFRA] modernise adaptation definitions

ad39e81

[INFRA] track CPO changes

adfe27f

[TEST] add test for alphabet customisation point

2c5356c

[cereal] switch to h-2 branch temporarily

bfca0d0

h-2 force-pushed the alph_refac branch from 281b8fb to bfca0d0 Compare May 10, 2019 14:32

rrahn approved these changes May 10, 2019

View reviewed changes

h-2 removed the request for review from smehringer May 10, 2019 17:42

h-2 merged commit e607fd3 into seqan:master May 10, 2019

h-2 deleted the alph_refac branch May 10, 2019 19:23

h-2 mentioned this pull request May 16, 2019

Add semialphabet any #981

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

second phase of alphabet restructuring #943

second phase of alphabet restructuring #943

h-2 commented Apr 30, 2019 •

edited

Loading

marehr commented Apr 30, 2019

h-2 commented May 3, 2019

smehringer left a comment

h-2 commented May 3, 2019

codecov bot commented May 7, 2019 •

edited

Loading

h-2 commented May 7, 2019

h-2 commented May 7, 2019

eseiler left a comment

rrahn left a comment

rrahn May 8, 2019

h-2 May 9, 2019

rrahn May 8, 2019

h-2 May 8, 2019

h-2 May 9, 2019

rrahn May 8, 2019

h-2 May 9, 2019

rrahn May 8, 2019

h-2 May 9, 2019

h-2 commented May 8, 2019

h-2 commented May 8, 2019

h-2 commented May 9, 2019

rrahn left a comment

rrahn May 9, 2019

h-2 May 9, 2019

h-2 May 10, 2019

rrahn May 9, 2019

h-2 May 9, 2019

rrahn May 9, 2019

h-2 May 9, 2019

h-2 commented May 9, 2019

h-2 commented May 10, 2019

h-2 commented May 10, 2019

rrahn left a comment

+                  }
+              }
+              }

second phase of alphabet restructuring #943

second phase of alphabet restructuring #943

Conversation

h-2 commented Apr 30, 2019 • edited Loading

marehr commented Apr 30, 2019

h-2 commented May 3, 2019

smehringer left a comment

Choose a reason for hiding this comment

h-2 commented May 3, 2019

codecov bot commented May 7, 2019 • edited Loading

Codecov Report

h-2 commented May 7, 2019

h-2 commented May 7, 2019

eseiler left a comment

Choose a reason for hiding this comment

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

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

h-2 commented May 8, 2019

h-2 commented May 8, 2019

h-2 commented May 9, 2019

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

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

h-2 commented May 9, 2019

h-2 commented May 10, 2019

h-2 commented May 10, 2019

rrahn left a comment

Choose a reason for hiding this comment

h-2 commented Apr 30, 2019 •

edited

Loading

codecov bot commented May 7, 2019 •

edited

Loading