[FEATURE] Node access for fm_index_cursor #2076

eseiler · 2020-08-27T08:46:28Z

Resolves #2061

codecov · 2020-08-27T10:14:52Z

Codecov Report

Merging #2076 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2076   +/-   ##
=======================================
  Coverage   97.95%   97.96%           
=======================================
  Files         264      264           
  Lines       10045    10050    +5     
=======================================
+ Hits         9840     9845    +5     
  Misses        205      205

Impacted Files	Coverage Δ
.../seqan3/search/fm_index/detail/fm_index_cursor.hpp	`100.00% <ø> (ø)`
include/seqan3/search/fm_index/fm_index_cursor.hpp	`100.00% <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 205674c...279273a. Read the comment docs.

marehr

I thought we wanted to expose the intervals; I'm not in favour for exposing the node. If we can, I would want to hide them. I'm marking this as WIP for now, because it is still not clear what the use case is.

CHANGELOG.md

marehr

Looks already pretty good, just one question

test/unit/search/fm_index_cursor/fm_index_cursor_collection_test_template.hpp

include/seqan3/search/fm_index/fm_index_cursor.hpp

marehr · 2020-09-08T08:44:07Z

Core Meeting 2020-09-08

We want a suffix_array_interval() function that returns a seqan3::suffix_array_interval which is an aggregate struct and has the two members left_bound and right_bound (of size_t) and behaves like a std::pair.

Document if half-open or not.

// Using half-open interval
struct suffix_array_interval
{
   size_t begin_position;
   size_t end_position;
};

// Using closed interval
struct suffix_array_interval
{
   size_t left_bound;
   size_t right_bound;
};

~~Use closed (which seems to be the way the sdsl has implemented this) if it can handle empty texts (and other corner cases). If it can't this is bad and the sdsl must fix it :(~~

We want to give half-open intervals back.

marehr

(Waiting on implementing the half-open interval accessor.)

eseiler · 2020-09-08T14:28:32Z

@marehr can you have a quick look at include/seqan3/search/fm_index/fm_index_cursor.hpp

If it is fine, I can start adjusting all the tests and documentation

marehr · 2020-09-08T16:46:53Z

@marehr can you have a quick look at include/seqan3/search/fm_index/fm_index_cursor.hpp

If it is fine, I can start adjusting all the tests and documentation

looks fine :) Or do you think that something seems odd?

eseiler · 2020-09-08T17:36:01Z

@marehr I also added a conversion operator to tuple ... it is nice for the use case where you want to reuse variables (see snippet, structured binding works with aggregate type, but std::tie needs the conversion)

open for discussion

auto [left_bound, right_bound] = cur.suffix_array_interval();
/// .. do some stuff with cursor
std::tie(left_bound, right_bound) = cur.suffix_array_interval();

marehr

Looks awesome :)

Do we really need the tuple thingy?

Did you find any instances where the interval can be empty?

marehr · 2020-09-15T14:13:07Z

include/seqan3/search/fm_index/fm_index_cursor.hpp

+    size_t end_position;
+
+    //!\brief Convert to tuple.
+    operator std::tuple<size_t &, size_t &>()


Suggested change

operator std::tuple<size_t &, size_t &>()

operator std::tuple<size_t &, size_t &>() &

I don't know, you can also do this:

struct foo { int a; int b; }; int main() { foo bar{1, 3}; auto & [a, b] = bar; b = 5; return bar.b; // has value 5 }

so we don't need a std::tie.

But what I want is

int main() { foo bar{1, 3}; auto & [a, b] = bar; b = 5; foo bar2{2, 4}; std::tie(a, b) = bar2; return bar.b; // has value 5 }

I'm not sure if this is really, really needed, but when I wrote the snippets, I assumed that it will work just to find out that it does not. Hence, I added it

I tried the &:

https://godbolt.org/z/4E6KcT

It doesn't work because .. ehm .. not so sure...
It would work if we the suffix_array_interval struct wasn't generated by another struct.

include/seqan3/search/fm_index/fm_index_cursor.hpp

eseiler · 2020-09-17T12:18:12Z

Did you find any instances where the interval can be empty?

No, the only thing that could happen is that you construct the cursor on an empty index.
Internally, closed intervals are used, so rb would underflow. But this was already a problem before...
I added an assert when constructing the cursor, such that it will assert if constructed on an empty index.

The most elegant solution would be to use half-open intervals in the FM-index, because then the cursor is always in a valid state (The cursor on an empty index would then use [0,0) instead of [0, size_t{-1}].)

rrahn · 2020-09-17T13:02:21Z

include/seqan3/search/fm_index/fm_index_cursor.hpp

+    size_t end_position;
+
+    //!\brief Convert to tuple.
+    operator std::tuple<size_t &, size_t &>()


Suggested change

operator std::tuple<size_t &, size_t &>()

operator std::tuple<size_t, size_t>() const noexcept

So this wouldn't work?

Because your version should require that this is an lvalue reference as well. Then the question remains, what is with the other versions const & should be also added. Move version are not required since we move/copy built-in types anyway.

Nope https://godbolt.org/z/j9avh9

rrahn

Looks good so far. Some minor change requrests.

rrahn · 2020-09-22T10:10:58Z

include/seqan3/search/fm_index/fm_index_cursor.hpp

@@ -32,6 +32,39 @@ namespace seqan3
 * \{
 */

+//!\brief The underlying suffix array intervals.


Suggested change

//!\brief The underlying suffix array intervals.

//!\brief The underlying suffix array interval.

rrahn · 2020-09-22T10:15:58Z

test/unit/search/fm_index_cursor/fm_index_cursor_collection_test_template.hpp

@@ -44,6 +43,10 @@ TYPED_TEST_P(fm_index_cursor_collection_test, ctr)
    // custom constructor
    TypeParam it0{fm};
    EXPECT_EQ(it0.query_length(), 0u);
+    if constexpr (!seqan3::bi_fm_index_specialisation<typename TypeParam::index_type>)


Could this not be part of the test fixture?

rrahn · 2020-09-22T10:18:40Z

test/unit/search/fm_index_cursor/fm_index_cursor_test_template.hpp

@@ -47,6 +46,10 @@ TYPED_TEST_P(fm_index_cursor_test, ctr)
    // custom constructor
    TypeParam it0{fm};
    EXPECT_EQ(it0.query_length(), 0u);
+    if constexpr (!seqan3::bi_fm_index_specialisation<typename TypeParam::index_type>)


same here. So I would call:

if constexpr (!TestFixture::is_bi_fm_index) { ... }

rrahn

Thanks a lot 👍

eseiler mentioned this pull request Aug 27, 2020

FM index cursor enable node access #2061

Closed

eseiler force-pushed the feature/node_access branch from 28fabf7 to 01bffc8 Compare August 27, 2020 08:49

eseiler requested review from a team and marehr and removed request for a team August 27, 2020 08:51

eseiler force-pushed the feature/node_access branch from 01bffc8 to aca183b Compare August 27, 2020 09:43

marehr reviewed Aug 27, 2020

View reviewed changes

marehr marked this pull request as draft August 27, 2020 12:56

eseiler force-pushed the feature/node_access branch from aca183b to 0f34bf6 Compare August 31, 2020 12:14

eseiler requested a review from marehr August 31, 2020 12:14

eseiler commented Aug 31, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

eseiler marked this pull request as ready for review August 31, 2020 12:17

marehr reviewed Sep 1, 2020

View reviewed changes

test/unit/search/fm_index_cursor/fm_index_cursor_collection_test_template.hpp Outdated Show resolved Hide resolved

include/seqan3/search/fm_index/fm_index_cursor.hpp Outdated Show resolved Hide resolved

eseiler requested a review from marehr September 2, 2020 08:11

marehr reviewed Sep 8, 2020

View reviewed changes

eseiler force-pushed the feature/node_access branch from 0f34bf6 to 3034c34 Compare September 8, 2020 14:25

eseiler requested a review from marehr September 8, 2020 14:28

eseiler removed the request for review from marehr September 8, 2020 16:47

eseiler force-pushed the feature/node_access branch from 3034c34 to ee29592 Compare September 8, 2020 17:33

eseiler requested a review from marehr September 8, 2020 17:36

eseiler force-pushed the feature/node_access branch from ee29592 to decfff9 Compare September 8, 2020 20:04

marehr reviewed Sep 15, 2020

View reviewed changes

eseiler force-pushed the feature/node_access branch from decfff9 to eb2c6fb Compare September 17, 2020 12:10

eseiler requested a review from marehr September 17, 2020 12:13

eseiler force-pushed the feature/node_access branch from eb2c6fb to 3dbcc99 Compare September 17, 2020 12:19

rrahn reviewed Sep 17, 2020

View reviewed changes

eseiler force-pushed the feature/node_access branch from 3dbcc99 to 8d5efe3 Compare September 18, 2020 11:48

marehr approved these changes Sep 18, 2020

View reviewed changes

marehr requested a review from rrahn September 18, 2020 12:14

rrahn requested changes Sep 22, 2020

View reviewed changes

eseiler added 2 commits September 22, 2020 12:39

[FEATURE] Suffix array interval of fm_index_cursor

7a8715f

Review

279273a

eseiler force-pushed the feature/node_access branch from 8d5efe3 to 279273a Compare September 22, 2020 10:39

eseiler requested a review from rrahn September 22, 2020 10:39

rrahn approved these changes Sep 22, 2020

View reviewed changes

eseiler merged commit ac39faa into seqan:master Sep 22, 2020

eseiler deleted the feature/node_access branch May 14, 2021 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Node access for fm_index_cursor #2076

[FEATURE] Node access for fm_index_cursor #2076

eseiler commented Aug 27, 2020

codecov bot commented Aug 27, 2020 •

edited

marehr left a comment

marehr left a comment

marehr commented Sep 8, 2020 •

edited

marehr left a comment •

edited

eseiler commented Sep 8, 2020

marehr commented Sep 8, 2020

eseiler commented Sep 8, 2020 •

edited

marehr left a comment

marehr Sep 15, 2020

marehr Sep 15, 2020

eseiler Sep 15, 2020 •

edited

eseiler Sep 17, 2020

eseiler commented Sep 17, 2020 •

edited

rrahn Sep 17, 2020

eseiler Sep 17, 2020

rrahn left a comment

rrahn Sep 22, 2020

rrahn Sep 22, 2020

rrahn Sep 22, 2020

rrahn left a comment

	operator std::tuple<size_t &, size_t &>()
	operator std::tuple<size_t &, size_t &>() &

	operator std::tuple<size_t &, size_t &>()
	operator std::tuple<size_t, size_t>() const noexcept

	//!\brief The underlying suffix array intervals.
	//!\brief The underlying suffix array interval.

[FEATURE] Node access for fm_index_cursor #2076

[FEATURE] Node access for fm_index_cursor #2076

Conversation

eseiler commented Aug 27, 2020

codecov bot commented Aug 27, 2020 • edited

Codecov Report

marehr left a comment

Choose a reason for hiding this comment

marehr left a comment

Choose a reason for hiding this comment

marehr commented Sep 8, 2020 • edited

Core Meeting 2020-09-08

marehr left a comment • edited

Choose a reason for hiding this comment

eseiler commented Sep 8, 2020

marehr commented Sep 8, 2020

eseiler commented Sep 8, 2020 • edited

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

eseiler Sep 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eseiler commented Sep 17, 2020 • edited

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

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

codecov bot commented Aug 27, 2020 •

edited

marehr commented Sep 8, 2020 •

edited

marehr left a comment •

edited

eseiler commented Sep 8, 2020 •

edited

eseiler Sep 15, 2020 •

edited

eseiler commented Sep 17, 2020 •

edited