-
Notifications
You must be signed in to change notification settings - Fork 48
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
Add findall
method
#218
Add findall
method
#218
Conversation
Codecov Report
@@ Coverage Diff @@
## master #218 +/- ##
==========================================
+ Coverage 88.45% 88.57% +0.12%
==========================================
Files 31 31
Lines 2408 2425 +17
==========================================
+ Hits 2130 2148 +18
+ Misses 278 277 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
4ff0244
to
6243900
Compare
6243900
to
f61662d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for contributing. Not sure about the signature though, it doesn't match Base
. What signature would you prefer?
Co-authored-by: Jakob Nybo Nissen <jakobnybonissen@gmail.com>
3e24fb2
to
fd0f57f
Compare
This PR is ready for another review pass. I've added a convenience method that uses a view to limit the search range and map indices back to a parent. If it's not worthwhile, it can be removed. BioSequences.jl/src/BioSequences.jl Lines 259 to 263 in fd0f57f
Also, is the PR code positioned where you'd like it to be within the project? |
Co-authored-by: Jakob Nybo Nissen <jakobnybonissen@gmail.com>
- Provides a convenience `findall` method that maps indices back to the parent sequence.
fd0f57f
to
7a16f9d
Compare
I think everything looks good now. |
Looking good! Did you want to add some docstrings and or a paragraph in the search section of the manual? No immediate pressure as they can always be added before a release. |
I reckon I can add some docstrings to this over the next couple of days. |
I'll just merge this, and then we can always add some docs later. |
I reckon this PR fulfils the
findall
method requirements.The
findall
method delegates to aSearch
iterator in the implementation. Let me know whether this approach suits and I'll add some docstrings.Closes #217.