[MISC] Add alphabet type template to fm_index #1222

eseiler · 2019-08-20T15:34:26Z

Partially reverts #1042

How to deprecate?
Add tests for new errors in cerealisation.

h-2 · 2019-08-20T15:51:21Z

I am against fm_index<text_t>, I would prefer fm_index<alph_t, layout>. The actual range types are entirely unimportant...

eseiler · 2019-08-20T16:07:00Z

I am against fm_index<text_t>, I would prefer fm_index<alph_t, layout>. The actual range types are entirely unimportant...

template<Semialphabet alph_t, text_layout text_layout_mode, detail::SdslIndex sdsl_index_type_ = default_sdsl_index_type>
fm_index{};

Then? (cerealising in will then be fm_index<dna4, text_layout::collection> index; ...)

But the question with how to deprecate still remains. (probably. If just alph_t is added, it might be easy)

But we can discuss tomorrow.

h-2 · 2019-08-20T16:40:30Z

Yes! We have so many things now that are changing, I don't think it's feasible to provide deprecation-marked update-paths at this point.

eseiler · 2019-08-21T09:21:51Z

@h-2 Like this?

include/seqan3/search/fm_index/bi_fm_index.hpp

doc/tutorial/read_mapper/read_mapper_step2.cpp

doc/tutorial/read_mapper/read_mapper_step3.cpp

doc/tutorial/read_mapper/read_mapper_step4.cpp

include/seqan3/search/fm_index/bi_fm_index.hpp

include/seqan3/search/fm_index/bi_fm_index_cursor.hpp

include/seqan3/search/fm_index/concept.hpp

test/unit/search/fm_index/fm_index_dna4_test.cpp

codecov · 2019-08-23T13:41:39Z

Codecov Report

❗ No coverage uploaded for pull request base (master@aa28a13). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1222   +/-   ##
=========================================
  Coverage          ?   96.83%           
=========================================
  Files             ?      217           
  Lines             ?     8654           
  Branches          ?        0           
=========================================
  Hits              ?     8380           
  Misses            ?      274           
  Partials          ?        0

Impacted Files	Coverage Δ
include/seqan3/search/algorithm/detail/search.hpp	`100% <ø> (ø)`
include/seqan3/search/algorithm/search.hpp	`78.94% <ø> (ø)`
.../seqan3/search/algorithm/detail/search_trivial.hpp	`86.11% <ø> (ø)`
...lude/seqan3/search/fm_index/bi_fm_index_cursor.hpp	`100% <100%> (ø)`
include/seqan3/search/fm_index/fm_index.hpp	`100% <100%> (ø)`
include/seqan3/search/fm_index/bi_fm_index.hpp	`100% <100%> (ø)`
include/seqan3/search/fm_index/fm_index_cursor.hpp	`100% <100%> (ø)`

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 aa28a13...179d26b. Read the comment docs.

marehr

One small change and I'll approve

include/seqan3/search/algorithm/detail/search.hpp

include/seqan3/search/fm_index/fm_index.hpp

test/unit/search/fm_index/fm_index_dna4_test.cpp

include/seqan3/search/fm_index/fm_index.hpp

include/seqan3/search/fm_index/concept.hpp

h-2 · 2019-08-29T09:36:22Z

CHANGELOG.md

+
+* **Changed class signature of (bi_)fm_index:**
+  All code that relies on automatic template deduction will be unaffected. In case you specified the template parameters
+  of an `seqan3::fm_index` or `seqan3::bi_fm_index` you will need to add the alphabet type as first parameter and pass a


Suggested change

of an `seqan3::fm_index` or `seqan3::bi_fm_index` you will need to add the alphabet type as first parameter and pass a

of a `seqan3::fm_index` or `seqan3::bi_fm_index` you will need to add the alphabet type as first parameter and pass a

h-2 · 2019-08-29T09:40:41Z

include/seqan3/search/fm_index/bi_fm_index.hpp

@@ -153,7 +143,7 @@ class bi_fm_index
    template <std::ranges::Range text_t>
    bi_fm_index(text_t && text)


You should now enforce that the text_t's alphabet type is convertible to the alpahbet type of the index, or not?

h-2 · 2019-08-29T09:42:54Z

include/seqan3/search/fm_index/bi_fm_index.hpp

-        //!\endcond
+    //!\cond
+        requires text_layout_mode == text_layout::single
+    //!\endcond
    void construct(text_t && text)


make this private or move into constructor

also document this in the changelog

h-2 · 2019-08-29T09:46:27Z

include/seqan3/search/fm_index/bi_fm_index_cursor.hpp

@@ -406,14 +408,16 @@ class bi_fm_index_cursor
     * No-throw guarantee.
     */
    template <Alphabet char_t>
+    //!\cond
+        requires ImplicitlyConvertibleTo<char_t, typename index_t::char_type>


we should use std::Convertible instead of "ImplicitlyConvertibleTo"

h-2 · 2019-08-29T09:47:52Z

include/seqan3/search/fm_index/fm_index.hpp

@@ -271,10 +252,10 @@ class fm_index
     * No guarantee. \if DEV \todo Ensure strong exception guarantee. \endif
     */
    template <std::ranges::Range text_t>
+    //!\cond
+        requires text_layout_mode == text_layout::single
+    //!\endcond
    void construct(text_t && text)


same comments as above (make private, constrain alphabet)

h-2 · 2019-08-29T09:48:56Z

include/seqan3/search/fm_index/fm_index.hpp

@@ -505,10 +485,25 @@ class fm_index
        text_begin_ss.set_vector(&text_begin);
        archive(text_begin_rs);
        text_begin_rs.set_vector(&text_begin);
+
+        auto sigma = alphabet_size<alphabet_t>;


above it is marked constexpr...

h-2 · 2019-08-29T09:50:06Z

include/seqan3/search/fm_index/fm_index.hpp

+        if (sigma != alphabet_size<alphabet_t>)
+        {
+            throw std::logic_error{"The fm_index was built over an alphabet of size " + std::to_string(sigma) +
+                                   " but it is being read into a fm_index with an alphabet of size " +


Suggested change

" but it is being read into a fm_index with an alphabet of size " +

" but it is being read into an fm_index with an alphabet of size " +

h-2 · 2019-08-29T09:50:15Z

include/seqan3/search/fm_index/fm_index.hpp

+        {
+            throw std::logic_error{std::string{"The fm_index was built over a "} +
+                                   (tmp ? "text collection" : "single text") +
+                                   " but it is being read into a fm_index expecting a " +


Suggested change

" but it is being read into a fm_index expecting a " +

" but it is being read into an fm_index expecting a " +

h-2 · 2019-08-29T09:50:56Z

include/seqan3/search/fm_index/fm_index_cursor.hpp

@@ -268,10 +262,12 @@ class fm_index_cursor
     * No-throw guarantee.
     */
    template <Alphabet char_t>
+    //!\cond
+        requires ImplicitlyConvertibleTo<char_t, typename index_t::char_type>


See above. Also I don't think you even need to require alphabet. Convertbility is all you need,

h-2 · 2019-08-29T15:04:57Z

include/seqan3/search/fm_index/fm_index_cursor.hpp

@@ -267,11 +261,13 @@ class fm_index_cursor
     *
     * No-throw guarantee.
     */
-    template <Alphabet char_t>
+    template <typename char_t>


Suggested change

template <typename char_t>

template <std::ConvertibleTo<typename index_t::char_type> char_t>

We usually prefer doing statis_asserts, no?

eseiler force-pushed the feature/return_of_the_alphabet branch from 9a989c1 to 97fda61 Compare August 21, 2019 09:20

eseiler force-pushed the feature/return_of_the_alphabet branch 2 times, most recently from 7af20e3 to 2b72bb9 Compare August 21, 2019 09:49

eseiler changed the title ~~[MISC] Revert to text_t template for indices~~ [MISC] Add alphabet type template to fm_index Aug 21, 2019

eseiler force-pushed the feature/return_of_the_alphabet branch 3 times, most recently from 04abc72 to 9f4d21f Compare August 21, 2019 13:17

marehr reviewed Aug 23, 2019

View reviewed changes

eseiler force-pushed the feature/return_of_the_alphabet branch from 9f4d21f to 0c63c4d Compare August 23, 2019 13:41

eseiler requested a review from marehr August 23, 2019 13:41

eseiler force-pushed the feature/return_of_the_alphabet branch 2 times, most recently from 9f27fd2 to f10e821 Compare August 26, 2019 09:03

marehr mentioned this pull request Aug 26, 2019

[FIX] visibility errors on gcc #1228

Closed

marehr reviewed Aug 28, 2019

View reviewed changes

eseiler requested a review from marehr August 28, 2019 12:47

eseiler force-pushed the feature/return_of_the_alphabet branch from f10e821 to 9b63a02 Compare August 28, 2019 12:47

marehr approved these changes Aug 28, 2019

View reviewed changes

eseiler force-pushed the feature/return_of_the_alphabet branch from 9b63a02 to 0bc3afb Compare August 28, 2019 16:33

eseiler requested a review from h-2 August 28, 2019 16:33

marehr approved these changes Aug 28, 2019

View reviewed changes

h-2 requested changes Aug 29, 2019

View reviewed changes

eseiler force-pushed the feature/return_of_the_alphabet branch from 0bc3afb to 83029f8 Compare August 29, 2019 11:11

eseiler requested a review from h-2 August 29, 2019 11:15

eseiler force-pushed the feature/return_of_the_alphabet branch from 83029f8 to 6a06dfe Compare August 29, 2019 13:03

h-2 reviewed Aug 29, 2019

View reviewed changes

[MISC] Add alphabet type template to fm_index

179d26b

eseiler force-pushed the feature/return_of_the_alphabet branch from 6a06dfe to 179d26b Compare August 29, 2019 15:13

eseiler requested a review from h-2 August 29, 2019 23:08

h-2 approved these changes Aug 30, 2019

View reviewed changes

h-2 merged commit 7dd3a6f into seqan:master Aug 30, 2019

eseiler deleted the feature/return_of_the_alphabet branch August 30, 2019 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MISC] Add alphabet type template to fm_index #1222

[MISC] Add alphabet type template to fm_index #1222

eseiler commented Aug 20, 2019 •

edited

Loading

h-2 commented Aug 20, 2019

eseiler commented Aug 20, 2019 •

edited

Loading

h-2 commented Aug 20, 2019

eseiler commented Aug 21, 2019

codecov bot commented Aug 23, 2019 •

edited

Loading

marehr left a comment

h-2 Aug 29, 2019

h-2 Aug 29, 2019

h-2 Aug 29, 2019

h-2 Aug 29, 2019

h-2 Aug 29, 2019

h-2 Aug 29, 2019

h-2 Aug 29, 2019

eseiler Aug 29, 2019

h-2 Aug 29, 2019

h-2 Aug 29, 2019

h-2 Aug 29, 2019

h-2 Aug 29, 2019

eseiler Aug 29, 2019

	of an `seqan3::fm_index` or `seqan3::bi_fm_index` you will need to add the alphabet type as first parameter and pass a
	of a `seqan3::fm_index` or `seqan3::bi_fm_index` you will need to add the alphabet type as first parameter and pass a

		@@ -153,7 +143,7 @@ class bi_fm_index
		template <std::ranges::Range text_t>
		bi_fm_index(text_t && text)

	" but it is being read into a fm_index with an alphabet of size " +
	" but it is being read into an fm_index with an alphabet of size " +

	" but it is being read into a fm_index expecting a " +
	" but it is being read into an fm_index expecting a " +

	template <typename char_t>
	template <std::ConvertibleTo<typename index_t::char_type> char_t>

[MISC] Add alphabet type template to fm_index #1222

[MISC] Add alphabet type template to fm_index #1222

Conversation

eseiler commented Aug 20, 2019 • edited Loading

h-2 commented Aug 20, 2019

eseiler commented Aug 20, 2019 • edited Loading

h-2 commented Aug 20, 2019

eseiler commented Aug 21, 2019

codecov bot commented Aug 23, 2019 • edited Loading

Codecov Report

marehr 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

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

eseiler commented Aug 20, 2019 •

edited

Loading

eseiler commented Aug 20, 2019 •

edited

Loading

codecov bot commented Aug 23, 2019 •

edited

Loading