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

[INFRA] Move seqan3::search from search/algorithm/ to search/. #1696

Merged

Conversation

Irallia
Copy link
Contributor

@Irallia Irallia commented Mar 26, 2020

Resolves #1695

@Irallia Irallia requested review from a team, eseiler and simonsasse and removed request for a team and eseiler March 26, 2020 15:11
@Irallia Irallia force-pushed the INFRA/search/move_search-algorithm_to_search branch from 9199be5 to f06c1e9 Compare March 30, 2020 09:07
@codecov
Copy link

codecov bot commented Mar 30, 2020

Codecov Report

Merging #1696 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1696      +/-   ##
==========================================
- Coverage   97.67%   97.65%   -0.03%     
==========================================
  Files         237      237              
  Lines        9045     9050       +5     
==========================================
+ Hits         8835     8838       +3     
- Misses        210      212       +2     
Impacted Files Coverage Δ
include/seqan3/search/detail/search.hpp 100.00% <ø> (ø)
...e/seqan3/search/detail/search_scheme_algorithm.hpp 93.82% <ø> (ø)
...seqan3/search/detail/search_scheme_precomputed.hpp 100.00% <ø> (ø)
include/seqan3/search/detail/search_trivial.hpp 100.00% <ø> (ø)
include/seqan3/search/search.hpp 82.75% <ø> (ø)
include/seqan3/search/search_result_range.hpp 100.00% <ø> (ø)

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 0fff98d...dfbd85c. Read the comment docs.

@Irallia Irallia force-pushed the INFRA/search/move_search-algorithm_to_search branch from f06c1e9 to 69e368c Compare March 30, 2020 09:08
@Irallia Irallia force-pushed the INFRA/search/move_search-algorithm_to_search branch from 69e368c to 9676fa7 Compare March 30, 2020 13:56
@Irallia Irallia requested review from a team and rrahn and removed request for a team March 30, 2020 13:56
Copy link
Contributor

@rrahn rrahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. Some changes regarding the documentation. I know it was copy'n'paste but we might as well just fix it here.

@@ -35,9 +35,56 @@
* SeqAn currently supports very fast FM indices. For more details visit the \ref submodule_fm_index
* "FM Index submodule".
*
* ## Search Algorithms
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* ## Search Algorithms
* ## Search algorithm

* The Search module offers a simple unified interface that allows searching FM indices and choosing the best
* algorithm based on the index at hand.
*
* ## FM Indices
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* ## FM Indices
* ## FM indices

*
* A search scheme is a collection of searches, where each search `s` specifies the order in which the blocks are
* searched (`s.pi`), the lower error bounds (`s.l`) and the upper error bounds (`s.u`). If the number of blocks that
* the query sequences are split into are known at compile time, the data structure `search` is recommended, otherwise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* the query sequences are split into are known at compile time, the data structure `search` is recommended, otherwise
* the query sequences are split into are known at compile time, the data structure seqan3::detail::search is recommended, otherwise

* represented as `std::vector` of seqan3::detail::search_dyn.
*
* All optimum search schemes are disjoint, i.e. no error configuration is covered by more than one search. Thus there
* is no need for a filtration phase after searching to remove duplicate hits. Allowing insertions and deletions will
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* is no need for a filtration phase after searching to remove duplicate hits. Allowing insertions and deletions will
* is no need for a filtration phase after searching to remove duplicate hits when searching with hamming distance. Allowing insertions and deletions will, however,

* the query sequences are split into are known at compile time, the data structure `search` is recommended, otherwise
* one has to use seqan3::detail::search_dyn. The first one implements its member variables as an an `std::array` of
* integers, the latter as an `std::vector` of integers.
* Similarily search schemes are defined. They are either implemented as an `std::array` of searches if the number of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Similarily search schemes are defined. They are either implemented as an `std::array` of searches if the number of
* Search schemes are defined similiar. They are either implemented as an `std::array` of searches if the number of

@rrahn
Copy link
Contributor

rrahn commented Mar 30, 2020

Also please add a API changelog entry.

Signed-off-by: Lydia Buntrock <lydia.buntrock@fu-berlin.de>
@Irallia Irallia requested a review from rrahn March 30, 2020 16:34
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

Successfully merging this pull request may close these issues.

Move seqan3::search from search/algorithm/ to just search/
3 participants