Skip to content
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

Design thoughts on algorithm configuration #1459

Closed
h-2 opened this issue Jan 2, 2020 · 3 comments
Closed

Design thoughts on algorithm configuration #1459

h-2 opened this issue Jan 2, 2020 · 3 comments

Comments

@h-2
Copy link
Member

h-2 commented Jan 2, 2020

Here are my 2¢:

  • Use function / function ptr syntax for hybrid options so mode<a::b> can be used as well as mode(a::b). Also for align_cfg::band. See Make mode() a runtime decision #1436
  • Things that are free to compute should not be configurable at all, e.g. score in alignment, index_cursor in search.
  • Avoid defining so many custom types for the arguments. seqan3::search_cfg::foo{seqan3::search_cfg::bar} is ugly (independent of whether or not bar is in extra namespace or in seqan3::). Alternatives are:
    • Use plain aggregate types for run-time options with multiple members. Provide copying member functions to offer "continuator" style access for GCC7. Promote use of designated initialisers for other compilers. See Design of error configurations #1458
    • "Continuator"-style can also be used for static config elements, e.g. seqan3::align_cfg::result::with_alignment()::with_front_coordinate().
    • Config elements could also appear like enums themselves, e.g. seqan3::align_cfg::mode::global vs seqan3::align_cfg::mode::local.
    • Just make things top-level config elements, e.g. for configuration of result type:
// not:
auto cfg = seqan3::align_cfg::result{seqan3::align_cfg::with_alignment,
                                     seqan3::align_cfg::with_front_coordinate};
// instead:
auto cfg = seqan3::align_cfg::with_alignment | 
           seqan3::align_cfg::with_front_coordinate

I think the last is even better than continuator-style, because it is not just about the result_type,
it actually affects the algorithm.

@h-2
Copy link
Member Author

h-2 commented Jan 2, 2020

Based on these design considerations and if we actual manage to avoid most parameter types, I am ok with putting those few parameter types into seqan3:: and not the config namespace (because they are not config elements!).

@smehringer
Copy link
Member

smehringer commented Mar 11, 2020

Just dumping this here and referencing #1639 as part of this discussion.

What we already agreed on by now is to reduce the number of custom types for config element argument since this is confusing.

So for all mutually exclusive config arguments, (e.g. those of align_cfg::result, align_cfg::mode, search_cfg::ouput, search_cfg::mode) we will just make things top-level config elements with the same internal id which automatically makes them not combinable by the config design. The doxygen documentation is confusing no matter what we do so there will always be the need for a landing page describing the possible configurations.
If possible/helpful, the config top-level elements will get the same prefix
e.g. align_cfg::with_alignment, align_cfg::with_front_coordinate already helps in identifying what belongs together.

See #1639 for the respective refactoring of search_cfg::mode.

In order to make these top-level config elements a dynamic choice, the proposal was to introduce a single search_cfg::dynamic_mode config element that is constructible from the static ones (see here for an example).

@rrahn
Copy link
Contributor

rrahn commented May 12, 2020

We have extensively discussed the design and this can be now closed. Here is the card with the resolution https://github.com/orgs/seqan/projects/4#card-31347419

@rrahn rrahn closed this as completed May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants