[FEATURE] search result struct #1706

smehringer · 2020-04-01T14:23:28Z

supersedes #1441
~~blocked by #1799~~

For the reviewer:
I split the changes into for logical commits because I think it makes it easier to review. Since the commits do not compiler without each other though, I would squash them in the end again.

To commit 1 (Unify cursor.locate() return value): In order to easily adapt the make_result function in the policy_result_builder, it was convenient to change the return value of cursor.locate() and I had in mind that we want to unify them both anyway. I only noticed the tail of tests I needed to adapt after everything was done. I'm sorry that this made the commit unnecessary heavy but we need this anyway.

Fixes #1434
Part of seqan/product_backlog#28

include/seqan3/search/algorithm/search_result.hpp

include/seqan3/search/detail/policy_result_builder.hpp

…nd adapt tests.

include/seqan3/search/fm_index/bi_fm_index_cursor.hpp

include/seqan3/search/fm_index/fm_index_cursor.hpp

eseiler · 2020-05-20T07:33:33Z

test/snippet/search/bi_fm_index.cpp

@@ -15,7 +15,7 @@ int main()
    cur.extend_left("AA"_dna4);                                         // search the pattern "AAGG"
    seqan3::debug_stream << "Number of hits: " << cur.count() << '\n';  // outputs: 2
    seqan3::debug_stream << "Positions in the genome: ";
-    for (auto const & pos : cur.locate())                               // outputs: 8, 22
+    for (auto const & pos : cur.locate())                               // outputs: (0, 8), (0, 22)


Do we want to promote const & or && in such a case?

hm.. I'm not sure 🤷‍♀️

In general, in a generic context we need to promote auto && since this encapsualtes true type deduction. In non-generic cases const & would be preferred since this is more explicit on what I can do on the type. Later it would be something like std::integral auto && pos. So since this is a generic case, we should promote the auto && version.

and in the other places below

include/seqan3/search/search_result.hpp

test/snippet/search/search.cpp

doc/tutorial/read_mapper/read_mapper_step4.cpp

doc/tutorial/search/index.md

Co-authored-by: Enrico Seiler <eseiler@users.noreply.github.com>

smehringer · 2020-05-20T12:43:25Z

note to myself: changelog entry!

doc/tutorial/read_mapper/read_mapper_step2.cpp

doc/tutorial/read_mapper/read_mapper_step3.cpp

doc/tutorial/read_mapper/read_mapper_step4.cpp

doc/tutorial/search/index.md

include/seqan3/search/search_result.hpp

rrahn · 2020-05-25T13:36:18Z

test/snippet/search/bi_fm_index.cpp

@@ -15,7 +15,7 @@ int main()
    cur.extend_left("AA"_dna4);                                         // search the pattern "AAGG"
    seqan3::debug_stream << "Number of hits: " << cur.count() << '\n';  // outputs: 2
    seqan3::debug_stream << "Positions in the genome: ";
-    for (auto const & pos : cur.locate())                               // outputs: 8, 22
+    for (auto const & pos : cur.locate())                               // outputs: (0, 8), (0, 22)


In general, in a generic context we need to promote auto && since this encapsualtes true type deduction. In non-generic cases const & would be preferred since this is more explicit on what I can do on the type. Later it would be something like std::integral auto && pos. So since this is a generic case, we should promote the auto && version.

rrahn · 2020-05-25T13:36:27Z

test/snippet/search/bi_fm_index.cpp

@@ -15,7 +15,7 @@ int main()
    cur.extend_left("AA"_dna4);                                         // search the pattern "AAGG"
    seqan3::debug_stream << "Number of hits: " << cur.count() << '\n';  // outputs: 2
    seqan3::debug_stream << "Positions in the genome: ";
-    for (auto const & pos : cur.locate())                               // outputs: 8, 22
+    for (auto const & pos : cur.locate())                               // outputs: (0, 8), (0, 22)


and in the other places below

test/unit/search/search_collection_test.cpp

test/unit/search/search_result_range_test.cpp

Co-authored-by: rrahn <rene.rahn@fu-berlin.de>

codecov · 2020-05-26T13:57:25Z

Codecov Report

Merging #1706 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1706      +/-   ##
==========================================
+ Coverage   97.52%   97.63%   +0.11%     
==========================================
  Files         248      249       +1     
  Lines        9439     9477      +38     
==========================================
+ Hits         9205     9253      +48     
+ Misses        234      224      -10

Impacted Files	Coverage Δ
include/seqan3/search/search.hpp	`88.63% <ø> (ø)`
...ude/seqan3/search/detail/policy_result_builder.hpp	`100.00% <100.00%> (ø)`
...lude/seqan3/search/fm_index/bi_fm_index_cursor.hpp	`100.00% <100.00%> (ø)`
include/seqan3/search/fm_index/fm_index_cursor.hpp	`100.00% <100.00%> (ø)`
include/seqan3/search/search_result.hpp	`100.00% <100.00%> (ø)`
include/seqan3/search/search_result_range.hpp	`100.00% <100.00%> (ø)`
include/seqan3/range/views/minimiser.hpp	`100.00% <0.00%> (ø)`
include/seqan3/core/algorithm/configuration.hpp	`100.00% <0.00%> (ø)`
...clude/seqan3/alignment/pairwise/align_pairwise.hpp	`100.00% <0.00%> (ø)`
...lude/seqan3/alignment/pairwise/alignment_range.hpp	`100.00% <0.00%> (ø)`
... and 13 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 f54cb5e...d29c234. Read the comment docs.

rrahn

looks in general fine to me. Only some minor questions left

rrahn · 2020-05-27T12:33:46Z

include/seqan3/search/search_result.hpp

 */
 template <typename query_id_type, typename cursor_type, typename reference_id_type, typename reference_begin_pos_type>
 class search_result
 {
+    static_assert(std::integral<query_id_type> || std::same_as<query_id_type, detail::empty_type>,
+                  "The query_id_type must either model std::integral or be detail::empty_type.");


Why can't this be simply template constraints above? What is the added value of these static asserts. It just writes out what it tests. I don't see this as a justification for making them static assert rather than class constraints?

I'm pretty sure agreed that we will use static asserts if the requires do not participate in overload resolution but I cannot find it in the core meeting notes.

I really don't see any benefit in these static asserts. Once we have support for the types in doxygen we can have a proper documentation. I really think that a static assert helps to identify hard to catch invariants. But I am in srong favour of constraining the primary templates for class templates, function templates and variable templates. Who knows when and who will overload it.

I changed it

rrahn · 2020-05-27T12:35:25Z

include/seqan3/search/configuration/all.hpp

@@ -60,6 +60,10 @@
 * | \ref search_configuration_subsection_hit_strategy "3. Hit"          |  ✅   |  ✅   |  ✅   |  ❌   |  ✅   |
 * | \ref seqan3::search_cfg::parallel "4: Parallel"                     |  ✅   |  ✅   |  ✅   |  ✅   |  ❌   |
 *
+ * \subsection search_configuration_search_result Search result type
+ *
+ * \copydetails seqan3::search_result


is this not misplaced here? Should this not be part of the search/all.hpp?

I thought it will help once the output configuration is also included but I can add it then.

Mhmm I see. I just wondering if there was another documentation of the seach module.

the search/all.hpp is very sparse and should proably be worked over anyway.

rrahn

great work. Thanks a lot.

rrahn · 2020-05-29T10:53:23Z

You can merge after rebaseing the commits.

smehringer · 2020-05-29T11:29:19Z

I will squash merge because the commits were not independent of each other (CI would not run though) and I think we should try to avoid this. Also all the changes would we really hard to include in clean commits.

rrahn reviewed Apr 3, 2020

View reviewed changes

include/seqan3/search/algorithm/search_result.hpp Outdated Show resolved Hide resolved

rrahn mentioned this pull request May 12, 2020

[FEATURE] Search result struct #1441

Closed

smehringer force-pushed the eseiler_search_result branch from 57758ef to 6de0ecd Compare May 15, 2020 09:33

marehr reviewed May 15, 2020

View reviewed changes

include/seqan3/search/detail/policy_result_builder.hpp Outdated Show resolved Hide resolved

smehringer force-pushed the eseiler_search_result branch 4 times, most recently from 3fff532 to c8e891b Compare May 19, 2020 19:50

smehringer added 2 commits May 20, 2020 07:15

[MISC] Unify cursor.locate() return value.

523125a

[FEATURE] Add seqan3::search_result.

0b6a0f6

smehringer force-pushed the eseiler_search_result branch from c8e891b to 3e6b5f8 Compare May 20, 2020 05:16

smehringer requested a review from eseiler May 20, 2020 05:24

smehringer added 2 commits May 20, 2020 08:27

[MISC,TEST] Incooporate seqan3::search_result into search algorithm a…

b42e3bf

…nd adapt tests.

[DOC] Adapt tutorial to seqan3::search_result

8e848f4

smehringer force-pushed the eseiler_search_result branch from 3e6b5f8 to 8e848f4 Compare May 20, 2020 06:27

eseiler requested changes May 20, 2020

View reviewed changes

smehringer and others added 2 commits May 20, 2020 14:14

Apply suggestions from code review

98dd742

Co-authored-by: Enrico Seiler <eseiler@users.noreply.github.com>

apply suggestions.

f087dcf

smehringer requested a review from eseiler May 20, 2020 12:24

eseiler approved these changes May 20, 2020

View reviewed changes

smehringer requested a review from rrahn May 20, 2020 14:24

rrahn requested changes May 25, 2020

View reviewed changes

smehringer and others added 3 commits May 26, 2020 07:43

Apply suggestions from code review

ee34882

Co-authored-by: rrahn <rene.rahn@fu-berlin.de>

apply suggestions.

146a2ca

fix header test

602dcfb

smehringer requested a review from rrahn May 26, 2020 11:17

fix documentation and jenkins error.

c690540

rrahn reviewed May 27, 2020

View reviewed changes

rrahn mentioned this pull request May 27, 2020

Search config/main dynamic hit config #1853

Merged

replace static asserts with actual requires clause.

d29c234

smehringer requested a review from rrahn May 28, 2020 12:45

rrahn approved these changes May 29, 2020

View reviewed changes

smehringer merged commit 4fcbeb8 into seqan:master May 29, 2020

smehringer deleted the eseiler_search_result branch May 29, 2020 11:31

This was referenced May 29, 2020

[NOP] Add Changelog entry for the seqan3::search_result. #1863

Merged

[Search] Investigate whether we can get rid of the index::text_layout (single vs collection) seqan/product_backlog#105

Open

rrahn mentioned this pull request Oct 12, 2020

3.0.2 Release #2001

Closed

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] search result struct #1706

[FEATURE] search result struct #1706

smehringer commented Apr 1, 2020 •

edited

Loading

eseiler May 20, 2020

smehringer May 20, 2020

rrahn May 25, 2020

rrahn May 25, 2020

smehringer commented May 20, 2020

rrahn May 25, 2020

rrahn May 25, 2020

codecov bot commented May 26, 2020 •

edited

Loading

rrahn left a comment

rrahn May 27, 2020

smehringer May 27, 2020

rrahn May 27, 2020

smehringer May 28, 2020

rrahn May 27, 2020

smehringer May 27, 2020

rrahn May 27, 2020

smehringer May 28, 2020

rrahn left a comment

rrahn commented May 29, 2020

smehringer commented May 29, 2020 •

edited

Loading

[FEATURE] search result struct #1706

[FEATURE] search result struct #1706

Conversation

smehringer commented Apr 1, 2020 • edited Loading

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

smehringer commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 26, 2020 • edited Loading

Codecov Report

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

rrahn left a comment

Choose a reason for hiding this comment

rrahn commented May 29, 2020

smehringer commented May 29, 2020 • edited Loading

smehringer commented Apr 1, 2020 •

edited

Loading

codecov bot commented May 26, 2020 •

edited

Loading

smehringer commented May 29, 2020 •

edited

Loading