[MISC] Refactor fm_index #1892

eseiler · 2020-06-10T18:37:12Z

Resolves #1542

codecov · 2020-06-11T17:17:53Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #1892   +/-   ##
=======================================
  Coverage   97.76%   97.77%           
=======================================
  Files         258      258           
  Lines        9685     9687    +2     
=======================================
+ Hits         9469     9471    +2     
  Misses        216      216

Impacted Files	Coverage Δ
...ix/detail/two_dimensional_matrix_iterator_base.hpp	`100.00% <100.00%> (ø)`
...ude/seqan3/range/detail/random_access_iterator.hpp	`100.00% <100.00%> (ø)`
include/seqan3/search/fm_index/bi_fm_index.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.hpp	`100.00% <100.00%> (ø)`
...clude/seqan3/range/views/enforce_random_access.hpp	`100.00% <0.00%> (ø)`
include/seqan3/range/views/kmer_hash.hpp	`94.65% <0.00%> (+0.08%)`	⬆️

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 35e15cd...214e23f. Read the comment docs.

marehr · 2020-06-15T09:56:54Z

auto genome = std::vector<"CGTAGGTCGATGC"_dna4, "CAAAA"_dna4>;
seqan3::bi_fm_index index{genome};

auto it = index.cursor(); // bidirectional cursor
it.extend_right("CGT"_dna4); // search CGT
seqan3::debug_stream << it.locate() << '\n'; // (0,0)

auto rev_it = it.to_rev_cursor();
seqan3::debug_stream << rev_it.locate() << '\n'; // (?,?)

Possible Outputs:

(1,10) old behaviour
(0,10) sequence indices are not reversed
(0,0) sequence indices are not reversed and text positions are from right to left

Open Questions:

Is this even used anywhere? Do we need it for some search implementation?
Does this need to be public?
to_rev_cursor.locate() is extremely slow (because sample size is non-existing)
- if locate is SO slow is it even worth to use this cursor?
- would this alternative definition of reverse_cursor be better?
  - reverse_cursor(text) = forward_cursor(reverse(text))

2020-06-15 Resolution:

Ask some people, if they don't need it or see an immediate use-case, otherwise throw it from the API.

Make it private in the mean-time and add a new issue.

2020-06-24 Resolution:

We remove the to_rev_interface since it is in the current state not stable or clearly defined what the expected behaviour should be.

CHANGELOG.md

test/unit/search/search_collection_test.cpp

include/seqan3/search/fm_index/bi_fm_index.hpp

include/seqan3/search/fm_index/fm_index.hpp

joergi-w · 2020-06-25T10:01:34Z

include/seqan3/search/fm_index/bi_fm_index.hpp

    void construct(text_t && text)
    {
        detail::fm_index_validator::validate<alphabet_t, text_layout_mode_>(text);

        fwd_fm = fm_index_type{text};
        rev_fm = rev_fm_index_type{text};
    }
-
+    


Here are spaces accidentally inserted 💅

include/seqan3/search/fm_index/fm_index.hpp

joergi-w

Thank you, looks good now! The most recent commit added some unintended spaces, which can be fixed together with the 2nd review.

smehringer

mostly some questions

include/seqan3/search/fm_index/bi_fm_index.hpp

smehringer · 2020-06-26T04:53:12Z

include/seqan3/search/fm_index/fm_index.hpp

-            tmp_text.back() = delimiter;
-
-        std::ranges::reverse(tmp_text);
+            std::ranges::reverse(tmp_text);


why are we reversing the text here in the if (!reverse) branch?

the reverse is whether the original text was reversed (reverse_fm_index).
If it's a "normal" fm_index, you can just reverse the test; if the text comes from the reverse_fm_index you have to do special stuff if there is only one item in the collection

smehringer · 2020-06-26T04:54:13Z

include/seqan3/search/fm_index/fm_index.hpp

+    }
+public:


Suggested change

}

public:

}

public:

smehringer · 2020-06-26T04:55:22Z

include/seqan3/search/fm_index/fm_index.hpp

@@ -274,51 +284,46 @@ class fm_index
    //!\cond
        requires (text_layout_mode_ == text_layout::collection)
    //!\endcond
-    void construct(text_t && text)
+    void construct(text_t && text, bool reverse = false)


why does this construct has a boolean for reversing in the normal fm_index when you have a whole class that can handle all the reversing?

It just handles the reversing of the text; the alternative would be to copy the complete code of the construct method to the reverse_fm_index and add the tiny if I need...

eseiler force-pushed the fix/search_bug branch 4 times, most recently from 6f7409b to ace6907 Compare June 10, 2020 20:24

eseiler force-pushed the fix/search_bug branch 2 times, most recently from 4fc615d to cd8abc5 Compare June 12, 2020 10:24

rrahn mentioned this pull request Jun 24, 2020

Suggestion from hannes concerning : seqan/seqan3#1542 seqan/product_backlog#145

Closed

5 tasks

eseiler force-pushed the fix/search_bug branch 2 times, most recently from 18f9539 to cbb0faa Compare June 24, 2020 17:47

eseiler marked this pull request as ready for review June 24, 2020 17:48

eseiler requested review from a team and joergi-w and removed request for a team June 24, 2020 17:48

eseiler force-pushed the fix/search_bug branch 2 times, most recently from 17a08aa to d2e2a8e Compare June 24, 2020 20:06

joergi-w requested changes Jun 24, 2020

View reviewed changes

eseiler requested a review from joergi-w June 25, 2020 09:26

joergi-w reviewed Jun 25, 2020

View reviewed changes

joergi-w approved these changes Jun 25, 2020

View reviewed changes

joergi-w requested review from a team and smehringer and removed request for a team June 25, 2020 10:10

smehringer requested changes Jun 26, 2020

View reviewed changes

eseiler requested a review from smehringer June 30, 2020 09:17

smehringer approved these changes Jun 30, 2020

View reviewed changes

eseiler added 3 commits June 30, 2020 11:41

[MISC] Create fm_index_validator

d7f4fdd

[FIX] Introduce detail::reverse_fm_index

acb1cb7

[MISC] rename {fwd.rev}_begin to {fwd,rev}_cursor

2ab92d2

eseiler added 2 commits June 30, 2020 11:42

[MISC] Remove reverse cursor interface

227fc82

[MISC] Add changelog entry

214e23f

eseiler force-pushed the fix/search_bug branch from c83a86a to 214e23f Compare June 30, 2020 09:42

smehringer merged commit 85469f8 into seqan:master Jun 30, 2020

eseiler deleted the fix/search_bug branch June 30, 2020 12:05

eseiler mentioned this pull request Jun 18, 2021

[MISC] Update to seqan 3.0.3 seqan/lambda#173

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MISC] Refactor fm_index #1892

[MISC] Refactor fm_index #1892

eseiler commented Jun 10, 2020 •

edited

Loading

codecov bot commented Jun 11, 2020 •

edited

Loading

marehr commented Jun 15, 2020 •

edited by rrahn

Loading

joergi-w Jun 25, 2020

joergi-w left a comment

smehringer left a comment

smehringer Jun 26, 2020

eseiler Jun 27, 2020

smehringer Jun 26, 2020

smehringer Jun 26, 2020

eseiler Jun 27, 2020

-    }
-public:
+    }
+public:

[MISC] Refactor fm_index #1892

[MISC] Refactor fm_index #1892

Conversation

eseiler commented Jun 10, 2020 • edited Loading

codecov bot commented Jun 11, 2020 • edited Loading

Codecov Report

marehr commented Jun 15, 2020 • edited by rrahn Loading

2020-06-15 Resolution:

2020-06-24 Resolution:

joergi-w Jun 25, 2020

Choose a reason for hiding this comment

joergi-w left a comment

Choose a reason for hiding this comment

smehringer left a comment

Choose a reason for hiding this comment

smehringer Jun 26, 2020

Choose a reason for hiding this comment

eseiler Jun 27, 2020

Choose a reason for hiding this comment

smehringer Jun 26, 2020

Choose a reason for hiding this comment

smehringer Jun 26, 2020

Choose a reason for hiding this comment

eseiler Jun 27, 2020

Choose a reason for hiding this comment

eseiler commented Jun 10, 2020 •

edited

Loading

codecov bot commented Jun 11, 2020 •

edited

Loading

marehr commented Jun 15, 2020 •

edited by rrahn

Loading