Skip to content

Commit

Permalink
Revert "Boundschecks for searchsorted & remove step(r::Range)==0 tests"
Browse files Browse the repository at this point in the history
This reverts commit 10e51e5.

Originally merged by 2d5dbf3, this is
incorrect in a number of ways.

The searchsorted* methods with explicit lo and hi indices are an not
part of the public API for these functions. Thus, the correctness of
the bounds given don't need to be checked – that is up to the caller.
If we decide to expose the ability to call searchsorted* with explicit
start and end indices, there should be a better API for that which
doesn't require passing an Ordering object and which checks bounds.

In particular, this broke the old behavior of searchsorted when the
value that's being searched for is not in the array. Fix #7916. Also
added somes tests to make sure this doesn't regress again.

At the time this was committed, I think that ranges could actually
have a zero step – there was a reason for that check to be there.
This may no longer be the case, but since it's cheap to check, we may
as well handle it correctly and decouple the correctness of the
sorting functions from a detail of the range implementation.

Conflicts:
	base/sort.jl
  • Loading branch information
StefanKarpinski committed Aug 8, 2014
1 parent 07b75b9 commit fda850b
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 12 deletions.
31 changes: 22 additions & 9 deletions base/sort.jl
Expand Up @@ -125,7 +125,6 @@ select(v::AbstractVector, k::Union(Int,OrdinalRange); kws...) = select!(copy(v),
# index of the first value of vector a that is greater than or equal to x;
# returns length(v)+1 if x is greater than all values in v.
function searchsortedfirst(v::AbstractVector, x, lo::Int, hi::Int, o::Ordering)
1 <= lo <= hi <= length(v) || throw(BoundsError())
lo = lo-1
hi = hi+1
@inbounds while lo < hi-1
Expand All @@ -142,7 +141,6 @@ end
# index of the last value of vector a that is less than or equal to x;
# returns 0 if x is less than all values of v.
function searchsortedlast(v::AbstractVector, x, lo::Int, hi::Int, o::Ordering)
1 <= lo <= hi <= length(v) || throw(BoundsError())
lo = lo-1
hi = hi+1
@inbounds while lo < hi-1
Expand All @@ -160,7 +158,6 @@ end
# if v does not contain x, returns a 0-length range
# indicating the insertion point of x
function searchsorted(v::AbstractVector, x, ilo::Int, ihi::Int, o::Ordering)
1 <= ilo <= ihi <= length(v) || throw(BoundsError())
lo = ilo-1
hi = ihi+1
@inbounds while lo < hi-1
Expand All @@ -179,21 +176,37 @@ function searchsorted(v::AbstractVector, x, ilo::Int, ihi::Int, o::Ordering)
end

function searchsortedlast{T<:Real}(a::Range{T}, x::Real, o::DirectOrdering)
n = max(min(iround((x-first(a))/step(a))+1,length(a)),1)
lt(o, x, a[n]) ? n-1 : n
if step(a) == 0
lt(o, x, first(a)) ? 0 : length(a)
else
n = max(min(iround((x-first(a))/step(a))+1,length(a)),1)
lt(o, x, a[n]) ? n-1 : n
end
end

function searchsortedfirst{T<:Real}(a::Range{T}, x::Real, o::DirectOrdering)
n = max(min(iround((x-first(a))/step(a))+1,length(a)),1)
lt(o, a[n] ,x) ? n+1 : n
if step(a) == 0
lt(o, first(a), x) ? length(a)+1 : 1
else
n = max(min(iround((x-first(a))/step(a))+1,length(a)),1)
lt(o, a[n] ,x) ? n+1 : n
end
end

function searchsortedlast{T<:Integer}(a::Range{T}, x::Real, o::DirectOrdering)
max(min(fld(ifloor(x)-first(a),step(a))+1,length(a)),0)
if step(a) == 0
lt(o, x, first(a)) ? 0 : length(a)
else
max(min(fld(ifloor(x)-first(a),step(a))+1,length(a)),0)
end
end

function searchsortedfirst{T<:Integer}(a::Range{T}, x::Real, o::DirectOrdering)
max(min(-fld(ifloor(-x)+first(a),step(a))+1,length(a)+1),1)
if step(a) == 0
lt(o, first(a), x) ? length(a)+1 : 1
else
max(min(-fld(ifloor(-x)+first(a),step(a))+1,length(a)+1),1)
end
end

searchsorted{T<:Real}(a::Range{T}, x::Real, o::DirectOrdering) =
Expand Down
6 changes: 3 additions & 3 deletions test/sorting.jl
Expand Up @@ -52,9 +52,9 @@ end
@test searchsorted(1:10, 1, by=(x -> x >= 5)) == searchsorted([1:10], 1, by=(x -> x >= 5))
@test searchsorted(1:10, 10, by=(x -> x >= 5)) == searchsorted([1:10], 10, by=(x -> x >= 5))

@test_throws BoundsError searchsortedfirst(1:10, 5, 0, 7, Base.Order.Forward)
@test_throws BoundsError searchsortedfirst(1:10, 5, 1, 11, Base.Order.Forward)
@test_throws BoundsError searchsortedfirst(1:10, 5, 5, 1, Base.Order.Forward)
@test searchsorted([], 0) == 1:0
@test searchsorted([1,2,3], 0) == 1:0
@test searchsorted([1,2,3], 4) == 4:3

a = rand(1:10000, 1000)

Expand Down

4 comments on commit fda850b

@simonster
Copy link
Member

Choose a reason for hiding this comment

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

One could argue that if this is not part of the public API, these functions should have a different name so that they aren't exported.

@StefanKarpinski
Copy link
Member Author

Choose a reason for hiding this comment

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

One could argue that – and I have when I was talking about separating the internal and external parts of interfaces. However, at the moment we do this all over the place, so this is hardly the only offender. We need a more general solution to this, even if only a best practice to give the internal parts of functions a different name.

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

No, at that point ranges already couldn't have a step of zero. See
#7078 (comment)

@StefanKarpinski
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, ok. Well, it still doesn't really hurt to check for that case, although I guess I could re-apply that part.

Please sign in to comment.