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

use old searchsorted() implementation until julia 1.11 #168

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name = "IntervalSets"
uuid = "8197267c-284f-5f27-9208-e0e47529a953"
version = "0.7.8"
version = "0.7.9"

[deps]
Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
Expand Down
78 changes: 66 additions & 12 deletions src/findall.jl
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,53 @@
5:14
```
"""
Base.findall(interval_d::Base.Fix2{typeof(in), <:Interval}, x::AbstractRange) =
searchsorted_interval(x, interval_d.x; rev=step(x) < zero(step(x)))
function Base.findall(interval_d::Base.Fix2{typeof(in),Interval{L,R,T}}, x::AbstractRange) where {L,R,T}
@static if VERSION < v"1.11-"
isempty(x) && return 1:0

interval = interval_d.x
il, ir = firstindex(x), lastindex(x)
δx = step(x)
if iszero(δx)
val = only(unique(x))
return val ∈ interval ? (firstindex(x):lastindex(x)) : (1:0)
end
a,b = if δx < zero(δx)
rev = findall(in(interval), reverse(x))
isempty(rev) && return rev

a = (il+ir)-last(rev)
b = (il+ir)-first(rev)

a,b
else
lx, rx = first(x), last(x)
l = max(leftendpoint(interval), lx - oneunit(δx))
r = min(rightendpoint(interval), rx + oneunit(δx))

(l > rx || r < lx) && return 1:0

a = il + max(0, round(Int, cld(l-lx, δx)))
a += (a ≤ ir && (x[a] == l && L == :open || x[a] < l))

b = min(ir, round(Int, cld(r-lx, δx)) + il)
b -= (b ≥ il && (x[b] == r && R == :open || x[b] > r))

a,b
end
# Reversing a range could change sign of values close to zero (cf
# sign of the smallest element in x and reverse(x), where x =
# range(BigFloat(-0.5),stop=BigFloat(1.0),length=10)), or more
# generally push elements in or out of the interval (as can cld),
# so we need to check once again.
a += +(a < ir && x[a] ∉ interval) - (il < a && x[a-1] ∈ interval)
b += -(il < b && x[b] ∉ interval) + (b < ir && x[b+1] ∈ interval)

a:b
else
searchsorted_interval(x, interval_d.x; rev=step(x) < zero(step(x)))
end
end

# We overload Base._findin to avoid an ambiguity that arises with
# Base.findall(interval_d::Base.Fix2{typeof(in),Interval{L,R,T}}, x::AbstractArray)
Expand Down Expand Up @@ -65,15 +110,24 @@
1:0
```
"""
function searchsorted_interval(X, i::Interval{L, R}; rev=false) where {L, R}
if rev === true
_searchsorted_begin(X, rightendpoint(i), Val(R); rev):_searchsorted_end(X, leftendpoint(i), Val(L); rev)
else
_searchsorted_begin(X, leftendpoint(i), Val(L); rev):_searchsorted_end(X, rightendpoint(i), Val(R); rev)
function searchsorted_interval end

if VERSION < v"1.11-"
searchsorted_interval(X, i::Interval{:closed, :closed}) = searchsortedfirst(X, leftendpoint(i)) :searchsortedlast(X, rightendpoint(i))
searchsorted_interval(X, i::Interval{:closed, :open}) = searchsortedfirst(X, leftendpoint(i)) :(searchsortedfirst(X, rightendpoint(i)) - 1)
searchsorted_interval(X, i::Interval{ :open, :closed}) = (searchsortedlast(X, leftendpoint(i)) + 1):searchsortedlast(X, rightendpoint(i))
searchsorted_interval(X, i::Interval{ :open, :open}) = (searchsortedlast(X, leftendpoint(i)) + 1):(searchsortedfirst(X, rightendpoint(i)) - 1)
Comment on lines +116 to +119
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we remove the keyword argument rev, that would be breaking. Could you add the keyword argument, or bump the version to v0.8.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rev support was added in #111, and it's exactly the change that causes performance regression

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the situation. If we revert #111, then we lose rev, and that must be considered as a breaking change. If you would like to avoid the breaking release, this PR should implement rev keyword argument without performance regression.

I'm okay with both choices: breaking release or adding the rev argument.

else
function searchsorted_interval(X, i::Interval{L, R}; rev=false) where {L, R}
if rev === true
_searchsorted_begin(X, rightendpoint(i), Val(R); rev):_searchsorted_end(X, leftendpoint(i), Val(L); rev)

Check warning on line 123 in src/findall.jl

View check run for this annotation

Codecov / codecov/patch

src/findall.jl#L121-L123

Added lines #L121 - L123 were not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is not tested on CI. Could you remove this # in CI.yml?

else
_searchsorted_begin(X, leftendpoint(i), Val(L); rev):_searchsorted_end(X, rightendpoint(i), Val(R); rev)

Check warning on line 125 in src/findall.jl

View check run for this annotation

Codecov / codecov/patch

src/findall.jl#L125

Added line #L125 was not covered by tests
end
end
end

_searchsorted_begin(X, x, ::Val{:closed}; rev) = searchsortedfirst(X, x; rev, lt=<)
_searchsorted_begin(X, x, ::Val{:open}; rev) = searchsortedlast(X, x; rev, lt=<) + 1
_searchsorted_end(X, x, ::Val{:closed}; rev) = searchsortedlast(X, x; rev, lt=<)
_searchsorted_end(X, x, ::Val{:open}; rev) = searchsortedfirst(X, x; rev, lt=<) - 1
_searchsorted_begin(X, x, ::Val{:closed}; rev) = searchsortedfirst(X, x; rev, lt=<)
_searchsorted_begin(X, x, ::Val{:open}; rev) = searchsortedlast(X, x; rev, lt=<) + 1
_searchsorted_end(X, x, ::Val{:closed}; rev) = searchsortedlast(X, x; rev, lt=<)
_searchsorted_end(X, x, ::Val{:open}; rev) = searchsortedfirst(X, x; rev, lt=<) - 1

Check warning on line 132 in src/findall.jl

View check run for this annotation

Codecov / codecov/patch

src/findall.jl#L129-L132

Added lines #L129 - L132 were not covered by tests
end