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 overflow in searchsorted*
#286
Conversation
Codecov Report
@@ Coverage Diff @@
## master #286 +/- ##
==========================================
- Coverage 95.90% 95.53% -0.37%
==========================================
Files 5 5
Lines 464 426 -38
==========================================
- Hits 445 407 -38
Misses 19 19
Continue to review full report at Codecov.
|
Hmmm, the failure on 1.0 is concerning. Do you know why that happens? |
Yes, should be fixed now. I had made a mistake in the test, but since empty ranges are equal in 1.6+, the test had passed. |
I feel like we're unnecessarily complicating matters here. We should simply pass the |
I think the codecov changes are ok, looks good to merge |
src/OffsetArrays.jl
Outdated
for f in [:searchsortedfirst, :searchsortedlast, :searchsorted] | ||
_safe_f = Symbol("_safe_" * String(f)) | ||
@eval function $_safe_f(v::OffsetArray, x, ilo, ihi, o::Base.Ordering) | ||
offset = firstindex(v) - firstindex(parent(v)) |
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.
What if v
is an offset vector of another offset vector? E.g., OffsetVector
of PaddedView
?
Edit: oh never mind, it seems to work just fine by delegating to the parent method.
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.
Maybe I'll add some tests, good to be sure
Fixes #285, but this doesn't fix all issues (as certain tests are broken). However, I think
searchsortedfirst
andsearchsortedlast
should be fixed.