RFC: Clean up search_sorted* functions

[kmsquire]

• Use 1-based indexing for searching subarrays
• Allow to be defined on AbstractVectors
• Make search_sorted and alias for search_sorted_first
• Round out functions with search_sorted_first_gt, search_sorted_last_lt (+ tests)
• Added documentation

Discussion

Recent commit df7a323 exposed a more general interface to search_sorted*() functions, allowing the caller to optionally specify the range to call. This interface was inconsistent between search_sorted() (1-based) and search_sorted_{first,last}() (0-based, range 0:length(a)+1). The first item above address this issue, using 1-based indexing; internally, 0-based indexing is still used (and this is the 'proper' way to do binary search--see http://www.tbray.org/ongoing/When/200x/2003/03/22/Binary)

Some additional deficiencies/idiosyncracies were identified (see df7a323600#commitcomment-2212563) and are addressed by this commit.

[ViralBShah]

This looks good. Any thoughts on how to avoid the _gt and _lt versions of the functions? We need a better way to specify comparisons in a generic way across all the search and sort functions.

@StefanKarpinski did a bunch of work on the comparison functions for sort.

[StefanKarpinski]

I'm not clear on that the _gt and _lt versions are for here.

[kmsquire]

We don't actually need to expose those functions, and they can also be
written easily in terms of the regular functions. I'll take them out here
and redefine them with timsort. The macro will probably be useful there as
well, but since there will be only two definitions here, I can remove it
for clarity for now. Thoughts?

On Wednesday, November 28, 2012, Stefan Karpinski wrote:

I'm not clear on that the _gt and _lt versions are for here.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1620#issuecomment-10805002.

[kmsquire]

Ok, I started thinking that search_sorted_first() and search_sorted_last() functions make more sense if you think about them as finding the first and last insertion point for x, and changed the documentation as such in this latest set of patches.

However, I just realized that that statement is not true for search_sorted_last(), because the last insertion point is the first value strictly greater than x, and the current definition is `>=x'.

I have a slight preference to change the definition of search_sorted_last() to reflect this, but I definitely would like some feedback on whether this is a good idea or not. The only in-tree user of this function is base/darray2.jl.

[ViralBShah]

I think search_sorted_last should insert at the last insertion point using >. If you can make that change, we can merge this.

Also, are you planning on committing timsort into base, or keeping it in a package?

[kmsquire]

With regard to changing the definition of search_sorted_last: I chickened out. The definition is actually arbitrary, and the in-tree use of search_sorted_last was correct with the old definition. So I changed the docs back.

While it's unlikely, now that we have packages, one other small possibility is that there's a package which uses this function. I could probably install all packages and look, but I need to get some sleep. ;-)

Assuming it passes Travis-CI, I think this is mergable for now.

For timsort, I'm about to push a WIP version.

[kmsquire]

Subsumed by #1691. Closing...

[kmsquire]

Except I forgot to add the documentation...