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

Loosen search iterator #221

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CiaranOMara
Copy link
Member

@CiaranOMara CiaranOMara commented Jan 30, 2022

This PR loosens the Search iterator to accommodate iterating in forward and reverse directions.

It needs a little more work to support the ApproxSearchQuery. It wasn't immediately obvious how to have the iterator generically include or pass through additional arguments to an underlying function. The other approach is to have ApproxSearchQuery own the additional argument for the number of allowed errors - comments would be helpful.

Let me know whether this seems like a good direction for a search iterator.

@CiaranOMara CiaranOMara marked this pull request as draft January 30, 2022 04:23
@codecov
Copy link

codecov bot commented Jan 30, 2022

Codecov Report

Merging #221 (a9d1355) into master (d3d8fc4) will decrease coverage by 0.18%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   88.57%   88.39%   -0.19%     
==========================================
  Files          31       31              
  Lines        2425     2430       +5     
==========================================
  Hits         2148     2148              
- Misses        277      282       +5     
Flag Coverage Δ
unittests 88.39% <50.00%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/BioSequences.jl 72.72% <50.00%> (-21.40%) ⬇️

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 d3d8fc4...a9d1355. Read the comment docs.

@TransGirlCodes
Copy link
Member

That seems very desirable to me, I remember thinking looking at the search code - "hmm how would you make Approx go backwards?".

@CiaranOMara
Copy link
Member Author

CiaranOMara commented Jun 6, 2022

The Search iterator would become generic or complete in its current state if all the queries owned their additional arguments. However, changing ApproxSearchQuery is a breaking change. So a design discussion is needed (hence converting this PR to draft).

I think we would be in a better position if we constrain our findnext and findprev method signatures to the following.

function findnext(query, seq::BioSequence, start::Integer)
    ...
end

function findprev(query, seq::BioSequence, start::Integer)
    ...
end

All the peripheral findfirst, findlast, findall methods can become generic if we adopt this constraint.

In terms of the ApproxSearchQuery, it would need to adopt the additional k argument somehow, and perhaps another wrapper/layer may be required to optimise for the reuse of ApproxSearchQuery's work.

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.

2 participants