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

[FIX] Missing queries in single element text collections #1316

Merged
merged 1 commit into from
Nov 5, 2019

Conversation

eseiler
Copy link
Member

@eseiler eseiler commented Oct 22, 2019

Resolves #1315

Copy link
Contributor

@svnbgnk svnbgnk left a comment

Choose a reason for hiding this comment

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

Nice, even with new unit test 👍 .

@eseiler eseiler requested a review from rrahn October 22, 2019 15:37
@codecov
Copy link

codecov bot commented Oct 22, 2019

Codecov Report

Merging #1316 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1316      +/-   ##
==========================================
+ Coverage   97.48%   97.48%   +<.01%     
==========================================
  Files         218      218              
  Lines        8680     8682       +2     
==========================================
+ Hits         8462     8464       +2     
  Misses        218      218
Impacted Files Coverage Δ
include/seqan3/search/fm_index/fm_index.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 6b3e868...bd0243d. Read the comment docs.

include/seqan3/search/fm_index/fm_index.hpp Outdated Show resolved Hide resolved
include/seqan3/search/fm_index/fm_index.hpp Outdated Show resolved Hide resolved
test/unit/search/search_collection_test.cpp Outdated Show resolved Hide resolved
test/unit/search/search_collection_test.cpp Outdated Show resolved Hide resolved
test/unit/search/search_collection_test.cpp Outdated Show resolved Hide resolved
Copy link
Member

@smehringer smehringer left a comment

Choose a reason for hiding this comment

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

Please add a note of this fix in the changelog

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.

some minor stuff

CHANGELOG.md Outdated
@@ -72,6 +72,8 @@ If possible, provide tooling that performs the changes, e.g. a shell-script.
## Notable Bug-fixes

* Copying and moving the `seqan3::fm_index` and `seqan3::bi_fm_index` now work properly.
* Searching in the `seqan3::fm_index` and `seqan3::bi_fm_index` constructed from a text collection containg a single
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
* Searching in the `seqan3::fm_index` and `seqan3::bi_fm_index` constructed from a text collection containg a single
* Searching in the `seqan3::fm_index` and `seqan3::bi_fm_index` constructed from a text collection containing a single

@@ -303,6 +304,10 @@ class fm_index
| views::join(delimiter)
| views::to<std::vector<uint8_t>>;

// we need at least one delimiter
if (std::ranges::distance(text) == 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

just always add the delimiter makes the code a little easier I guess. And it shouldn't hurt.

Copy link
Member Author

Choose a reason for hiding this comment

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

Somewhere, something breaks 🤷‍♂

@rrahn
Copy link
Contributor

rrahn commented Oct 25, 2019

ci is broken?

@eseiler eseiler force-pushed the fix/fm branch 5 times, most recently from 638751a to ae89ae9 Compare October 30, 2019 12:13
@eseiler
Copy link
Member Author

eseiler commented Oct 30, 2019

@rrahn Travis runs through, but won't tell us here.
(Note that I changed some stuff back since it had some unforeseen side effects...)

@rrahn rrahn merged commit 739d509 into seqan:master Nov 5, 2019
@eseiler eseiler deleted the fix/fm branch November 22, 2019 10:03
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.

Search on bi_fm_index doesn't find all matches
4 participants