From 5006db1399ed521f00f56708181879627ad39d5b Mon Sep 17 00:00:00 2001 From: Alexander Plavin Date: Sat, 30 Sep 2023 11:17:14 -0400 Subject: [PATCH] improve performance of searchsorted(range, lt=<) (#50365) No behavior change, just a performance improvement: searchsorted(range, lt=<) now performs exactly as fast as searchsorted(range). Passing lt=< allows searchsorted to find floating point values such as -0.0, so it's useful to make it fast - see https://github.com/JuliaLang/julia/issues/44102#issuecomment-1418152295. --- base/ordering.jl | 21 +++++++++------------ base/sort.jl | 17 ++++++++++------- test/ordering.jl | 25 ++++++++++++++++++------- test/sorting.jl | 10 ++++++++++ 4 files changed, 47 insertions(+), 26 deletions(-) diff --git a/base/ordering.jl b/base/ordering.jl index 5383745b1dd1f..13f5eceb2d4dc 100644 --- a/base/ordering.jl +++ b/base/ordering.jl @@ -125,18 +125,15 @@ lt(o::Lt, a, b) = o.lt(a,b) (lt(p.order, da, db)::Bool) | (!(lt(p.order, db, da)::Bool) & (a < b)) end -_ord(lt::typeof(isless), by::typeof(identity), order::Ordering) = order -_ord(lt::typeof(isless), by, order::Ordering) = By(by, order) - -function _ord(lt, by, order::Ordering) - if order === Forward - return Lt((x, y) -> lt(by(x), by(y))) - elseif order === Reverse - return Lt((x, y) -> lt(by(y), by(x))) - else - error("Passing both lt= and order= arguments is ambiguous; please pass order=Forward or order=Reverse (or leave default)") - end -end + +_ord(lt::typeof(isless), by, order::Ordering) = _by(by, order) +_ord(lt::typeof(isless), by, order::ForwardOrdering) = _by(by, order) # disambiguation +_ord(lt::typeof(isless), by, order::ReverseOrdering{ForwardOrdering}) = _by(by, order) # disambiguation +_ord(lt, by, order::ForwardOrdering) = _by(by, Lt(lt)) +_ord(lt, by, order::ReverseOrdering{ForwardOrdering}) = reverse(_by(by, Lt(lt))) +_ord(lt, by, order::Ordering) = error("Passing both lt= and order= arguments is ambiguous; please pass order=Forward or order=Reverse (or leave default)") +_by(by, order::Ordering) = By(by, order) +_by(::typeof(identity), order::Ordering) = order """ ord(lt, by, rev::Union{Bool, Nothing}, order::Ordering=Forward) diff --git a/base/sort.jl b/base/sort.jl index 59ab1ea7413fc..ceea7365a08b2 100644 --- a/base/sort.jl +++ b/base/sort.jl @@ -225,7 +225,10 @@ function searchsorted(v::AbstractVector, x, ilo::T, ihi::T, o::Ordering)::UnitRa return (lo + 1) : (hi - 1) end -function searchsortedlast(a::AbstractRange{<:Real}, x::Real, o::DirectOrdering)::keytype(a) + +const FastRangeOrderings = Union{DirectOrdering,Lt{typeof(<)},ReverseOrdering{Lt{typeof(<)}}} + +function searchsortedlast(a::AbstractRange{<:Real}, x::Real, o::FastRangeOrderings)::keytype(a) require_one_based_indexing(a) f, h, l = first(a), step(a), last(a) if lt(o, x, f) @@ -238,7 +241,7 @@ function searchsortedlast(a::AbstractRange{<:Real}, x::Real, o::DirectOrdering): end end -function searchsortedfirst(a::AbstractRange{<:Real}, x::Real, o::DirectOrdering)::keytype(a) +function searchsortedfirst(a::AbstractRange{<:Real}, x::Real, o::FastRangeOrderings)::keytype(a) require_one_based_indexing(a) f, h, l = first(a), step(a), last(a) if !lt(o, f, x) @@ -251,7 +254,7 @@ function searchsortedfirst(a::AbstractRange{<:Real}, x::Real, o::DirectOrdering) end end -function searchsortedlast(a::AbstractRange{<:Integer}, x::Real, o::DirectOrdering)::keytype(a) +function searchsortedlast(a::AbstractRange{<:Integer}, x::Real, o::FastRangeOrderings)::keytype(a) require_one_based_indexing(a) f, h, l = first(a), step(a), last(a) if lt(o, x, f) @@ -259,7 +262,7 @@ function searchsortedlast(a::AbstractRange{<:Integer}, x::Real, o::DirectOrderin elseif h == 0 || !lt(o, x, l) length(a) else - if o isa ForwardOrdering + if !(o isa ReverseOrdering) fld(floor(Integer, x) - f, h) + 1 else fld(ceil(Integer, x) - f, h) + 1 @@ -267,7 +270,7 @@ function searchsortedlast(a::AbstractRange{<:Integer}, x::Real, o::DirectOrderin end end -function searchsortedfirst(a::AbstractRange{<:Integer}, x::Real, o::DirectOrdering)::keytype(a) +function searchsortedfirst(a::AbstractRange{<:Integer}, x::Real, o::FastRangeOrderings)::keytype(a) require_one_based_indexing(a) f, h, l = first(a), step(a), last(a) if !lt(o, f, x) @@ -275,7 +278,7 @@ function searchsortedfirst(a::AbstractRange{<:Integer}, x::Real, o::DirectOrderi elseif h == 0 || lt(o, l, x) length(a) + 1 else - if o isa ForwardOrdering + if !(o isa ReverseOrdering) cld(ceil(Integer, x) - f, h) + 1 else cld(floor(Integer, x) - f, h) + 1 @@ -283,7 +286,7 @@ function searchsortedfirst(a::AbstractRange{<:Integer}, x::Real, o::DirectOrderi end end -searchsorted(a::AbstractRange{<:Real}, x::Real, o::DirectOrdering) = +searchsorted(a::AbstractRange{<:Real}, x::Real, o::FastRangeOrderings) = searchsortedfirst(a, x, o) : searchsortedlast(a, x, o) for s in [:searchsortedfirst, :searchsortedlast, :searchsorted] diff --git a/test/ordering.jl b/test/ordering.jl index 547d8d8dd0e8b..972d48c17b1af 100644 --- a/test/ordering.jl +++ b/test/ordering.jl @@ -2,21 +2,24 @@ using Test -import Base.Order: Forward, Reverse +import Base.Order: Forward, Reverse, ord, Lt, By, ReverseOrdering # every argument can flip the integer order by passing the right value. Here, # we enumerate a few of these combinations and check that all these flips # compound so that in total we either have an increasing or decreasing sort. for (s1, rev) in enumerate([true, false]) - for (s2, lt) in enumerate([>, <, (a, b) -> a - b > 0, (a, b) -> a - b < 0]) + for (s2, lt) in enumerate([(a, b)->isless(b, a), isless, >, <, (a, b) -> a - b > 0, (a, b) -> a - b < 0]) for (s3, by) in enumerate([-, +]) for (s4, order) in enumerate([Reverse, Forward]) - if iseven(s1 + s2 + s3 + s4) - target = [1, 2, 3] - else - target = [3, 2, 1] - end + is_fwd = iseven(s1 + s2 + s3 + s4) + target = is_fwd ? (1:3) : (3:-1:1) + # arrays, integer and float ranges sometimes have different code paths @test target == sort([2, 3, 1], rev=rev, lt=lt, by=by, order=order) + + @test target == sort(1:3, rev=rev, lt=lt, by=by, order=order) + @test target == sort(3:-1:1, rev=rev, lt=lt, by=by, order=order) + @test float(target) == sort(1.0:3, rev=rev, lt=lt, by=by, order=order) + @test float(target) == sort(3.0:-1:1, rev=rev, lt=lt, by=by, order=order) end end end @@ -40,3 +43,11 @@ struct SomeOtherOrder <: Base.Order.Ordering end @test reverse(Forward) === Reverse @test reverse(Reverse) === Forward + +@test ord(isless, identity, false, Forward) === Forward +@test ord(isless, identity, true, Forward) === Reverse +@test ord(<, identity, false, Forward) === Lt(<) +@test ord(isless, abs, false, Forward) === By(abs) +@test ord(<, abs, false, Forward) === By(abs, Lt(<)) +@test ord(<, abs, true, Forward) === ReverseOrdering(By(abs, Lt(<))) +@test ord(<, abs, true, Reverse) === By(abs, Lt(<)) diff --git a/test/sorting.jl b/test/sorting.jl index 147a70a5db7d9..f89291c8a685a 100644 --- a/test/sorting.jl +++ b/test/sorting.jl @@ -1225,6 +1225,16 @@ end @test searchsorted(v, 0.1, rev=true) === 4:3 end end + + @testset "ranges issue #44102, PR #50365" begin + # range sorting test for different Ordering parameter combinations + @test searchsorted(-1000.0:1:1000, -0.0) === 1001:1000 + @test searchsorted(-1000.0:1:1000, -0.0; lt=<) === 1001:1001 + @test searchsorted(-1000.0:1:1000, -0.0; lt=<, by=x->x) === 1001:1001 + @test searchsorted(reverse(-1000.0:1:1000), -0.0; lt=<, by=-) === 1001:1001 + @test searchsorted(reverse(-1000.0:1:1000), -0.0, rev=true) === 1002:1001 + @test searchsorted(reverse(-1000.0:1:1000), -0.0; lt=<, rev=true) === 1001:1001 + end end # The "searchsorted" testset is at the end of the file because it is slow.