added string radix sort and sortperm + tests #27

xiaodaigh · 2018-01-03T09:04:16Z

I have added StringRadixSortAlg so that (LSD) radix sort can be applied to strings

stevengj · 2018-01-03T15:51:46Z

src/string_radix_sort.jl

+The default is `UInt`, so on a 64 bit machine it loads 64 bits (8 bytes) at a time.
+If the `String` is shorter than 8 bytes then it's padded with 0. 
+
+Assumes machine is little-endian


Is that true even with the ntoh calls?

It's not true. Have removed

stevengj · 2018-01-03T15:55:48Z

src/string_radix_sort.jl

+        return ntoh(unsafe_load(Ptr{T}(pointer(s, skipbytes+1))))
+    else
+        ns = (sizeof(T) - min(sizeof(T), n - skipbytes))*8
+        h = unsafe_load(Ptr{T}(pointer(s, skipbytes+1)))


Couldn't this segfault if you read past the end of the string? e.g. if the string ends on a page boundary and the next page is inaccessible?

made a segfault safe version

stevengj · 2018-01-03T15:58:01Z

src/string_radix_sort.jl

+        h = unsafe_load(Ptr{T}(pointer(s, skipbytes+1)))
+        h = h << ns
+        h = h >> ns
+        return ntoh(h)


Wouldn't it be better to do return (ntoh(unsafe_load(...)) >> ns) << ns, i.e. do ntoh first and flip the order of the shifts, so that it still works on bigendian machines?

yep. but supplanted by the segfault safe version now

stevengj · 2018-01-03T16:03:03Z

src/string_radix_sort.jl

+    if lo >= hi;  return svec;  end
+
+    # find the maximum string length
+    lens = reduce((x,y) -> max(x,sizeof(y)),0, svec)


Just use maximum(sizeof, svec)

stevengj · 2018-01-03T16:04:08Z

src/string_radix_sort.jl

+- `s`:          a `String`
+- `skipbytes`:  how many bytes to skip e.g. load_bits("abc", 1) will load "bc" as bits
+"""
+load_bits(s::String, skipbytes = 0) = load_bits(UInt, s, skipbytes)


Not clear why this method is needed, since you always pass a type to load_bits below

have removed

stevengj · 2018-01-03T16:07:39Z

src/string_radix_sort.jl

+        # are all values the same at this radix?
+        if bin[idx,j] == len;  continue;  end
+
+        cbin = cumsum(bin[:,j])


Maybe use cumsum! with a preallocated array. Note that bin[:,j] also allocates a copy. You might want to just write a loop to do the sum with no copies.

stevengj · 2018-01-03T16:13:41Z

src/string_radix_sort.jl

+Currently radixsot 
+"""
+function sortperm_radixsort(svec::AbstractVector{String}; rev::Union{Bool,Void}=nothing, order::Ordering=Forward)
+    siv = StringIndexVector(copy(svec), collect(1:length(svec)))


Usually you would define a sortperm_radixsort! function that doesn't make a copy of svec, and then define sortperm_radixsort(svec, ...) = sortperm_radixsort!(copy(svec), ...)

This reverts commit 5b4ac54.

…g_radixsort2

…to string_radixsort2" This reverts commit 9264bb3.

xiaodaigh · 2018-01-04T03:07:36Z

The speed advantage of radixsort vs Base.sort seems to have vanished after implementing the segfault safe version. The performance is so bad that it's better to wait till I come up with a performant version.

stevengj · 2018-01-04T15:44:11Z

src/string_radix_sort.jl

+
+function load_bits_with_padding(::Type{UInt128}, s::String, skipbytes = 0)  
+    n = sizeof(s)
+    T = UInt128


I'm not sure the compiler is smart enough to constant-fold a type that you assign to a local variable like this. Does it improve performance if you just paste UInt128 directly in the code?

(I think it should be possible to write this function in a way that works for all T simultaneously with no penalty by making sure the branches get optimized out, but easier to optimize the case for a single T first.)

Ok, I will just replace wiht
Actually I think a macro will work better here; just generate the code for all types. It's basically the same anyway. Might look into that.

I actually initial I have one function for all types. But it keeps on crashing. I think it has to do with the Base.zext_int(UInt64, ::UInt128) which doesn't make sense; even that branch will never be reached, the compiler still complained and crashed the program every time.

stevengj · 2018-01-04T15:53:56Z

src/string_radix_sort.jl

+            if o == Reverse
+                sorttwo!(.~load_bits.(UInt128, svec, skipbytes), svec)
+            else
+                sorttwo!(load_bits.(UInt128, svec, skipbytes), svec)


Note that this allocates arrays over and over; would be better to allocate a bits array once and overwrite it in-place with bits .= load_bits.(...)

xiaodaigh · 2018-01-04T21:43:46Z

more comprehensive test cases to come to test all branches

minor = removed lexicographic ordering as it's not longer in 0.7

xiaodaigh · 2018-01-12T15:08:30Z

Bump!

String radix sort is 3x faster than base sort and passes all tests.

using SortingAlgorithms
x = rand([randstring(8) for i=1:1_000_000], 100_000_00);
@time sort(x) |> issorted #11s
@time sort(x, alg=StringRadixSort) |> issorted # 3.7s

nalimilan · 2018-01-12T17:56:31Z

Any reason why this cannot be just the String implementation of the RadixSort algorithm, rather than a separate StringRadixSort one?

xiaodaigh · 2018-01-13T02:04:03Z

@nalimilan I tried to use RadixSort to start with but then I found that sort doesn't dispatch on alg, so I can't send it down the path I want. One can solve this by overloading sort! with this signiture sort!(x::Vector{String}, ..., ::RadixSort). So that can be solved.

But the real issue is sortperm as that function by default transforms the data using the Perm ordering and calls sort!(indexes(x),...Perm(x), RadixSort) so it goes down the RadixSort function! But in that function it will fail because it will try to sort order.data which is of string and not bits type so it fails! The only way to solve this is to overload sort!(x,....,RadixSort, o::Base.Perm{String}) but I believe there is no Perm{String} type. So the only way to solve this is to overload sortperm(x::Vector{String}). But I think the idea is not to overload sortperm and sort but to overload the ! function.

nalimilan · 2018-01-13T11:06:07Z

Sorry, I'm not familiar with that machinery, but these implementation issues should definitely be fixed in some way. It would be too bad to have RadixSort fail for string vectors and require people to choose a different algorithm manually just because of implementation problems. For example, it could be useful for StatsBase.countmap to have RadixSort support strings.

xiaodaigh · 2018-01-13T12:33:08Z

I think a way to solve it is some kind of method-piracy. Can overload the sort method in Base with sort(Vector{String}) and then check if alg == RadixSort, if it is then apply the fast string algorithm if not then call Base.sort. But notice there is no sort method in SortingAlgorithms.jl atm, so this will be an "innovation".

nalimilan · 2018-01-13T12:40:32Z

Hmm, that wouldn't be great. Maybe we need to change something in Base so that it's possible to dispatch on the algorithm?

xiaodaigh · 2018-01-13T12:57:47Z

I can't find it now but I was involved in a discussion, basically, either base adds in a sortperm(x,alg) signature or Julia can dispatch on named arguments so we can overload sortperm(x::Vector{String}, alg=RadixSort). If not then sortperm() calls sort!(x, alg = RadixSort) which will fail as there is no RadixSort method for Strings.

nalimilan · 2018-01-13T14:37:10Z

Maybe JuliaLang/julia#24770?

Anyway, I'd say it would be fine to make a PR against Base to add sortperm(x,alg). Could even happen before 0.7 if that's not too disruptive.

xiaodaigh · 2018-01-13T21:17:49Z

I recall Chris R saying that the real fix is to dispatch on named arguments... can't find that discussion now.

nalimilan · 2018-01-14T10:48:23Z

You mean on keyword arguments? That won't happen in 1.0, and maybe never, so we'd better find a solution until then... :-)

nalimilan · 2018-01-28T09:16:44Z

Could you file an issue in Base to discuss the problem with sortperm?

ViralBShah · 2020-05-25T18:30:37Z

Is this something we want to try and get merged?

nalimilan · 2020-07-09T14:57:12Z

src/string_radix_sort.jl

+"""
+    sorttwo!(vs, index)
+
+Sort both the `vs` and reorder `index` at the same. This allows for faster sortperm


This sounds similar to what I experimented at #33.

nalimilan · 2020-09-27T14:19:09Z

Why close this? Can we try to find a way forward by discussing the needed changes in Base?

added string radix sort and sortperm + tests

d5584e4

stevengj reviewed Jan 3, 2018

View reviewed changes

xiaodaigh added 7 commits January 4, 2018 10:58

incorporated comments

9663e53

more incorporation of comments

f6f912b

towards segfault free implementation

5b4ac54

Revert "towards segfault free implementation"

905df65

This reverts commit 5b4ac54.

segfault safe load_bits

5af5730

Merge remote-tracking branch 'xiaodaigh/string_radixsort2' into strin…

9264bb3

…g_radixsort2

Revert "Merge remote-tracking branch 'xiaodaigh/string_radixsort2' in…

e81cff3

…to string_radixsort2" This reverts commit 9264bb3.

xiaodaigh added 2 commits January 4, 2018 14:53

solved performance issue; need to improve code length with macros

6d524ad

fixed load_bits bug

4dc9985

stevengj reviewed Jan 4, 2018

View reviewed changes

xiaodaigh added 6 commits January 5, 2018 08:56

less allocation of arrays

cef488b

updated with boundary check algorithms: comprehensive tests to come

348d081

updated load_bits function to be more performant and safe + tests +minor

5b183bd

minor = removed lexicographic ordering as it's not longer in 0.7

minor update to stringsort tests

6f8ad4a

minor bug fix + do not use UInt128 for bits loading for performance

5e239a5

updated readme

0d5111e

got rid of radixsort signature

3307017

xiaodaigh mentioned this pull request Dec 16, 2018

There is another similar package: SortingLab.jl #31

Closed

nalimilan reviewed Jul 9, 2020

View reviewed changes

xiaodaigh closed this Sep 27, 2020

LilithHafner added the new algorithm label Dec 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added string radix sort and sortperm + tests #27

added string radix sort and sortperm + tests #27

xiaodaigh commented Jan 3, 2018

stevengj Jan 3, 2018

xiaodaigh Jan 4, 2018

stevengj Jan 3, 2018

xiaodaigh Jan 4, 2018

stevengj Jan 3, 2018

xiaodaigh Jan 4, 2018

stevengj Jan 3, 2018

stevengj Jan 3, 2018

xiaodaigh Jan 4, 2018

stevengj Jan 3, 2018

xiaodaigh Jan 4, 2018

stevengj Jan 3, 2018

xiaodaigh Jan 4, 2018

xiaodaigh commented Jan 4, 2018 •

edited

Loading

stevengj Jan 4, 2018

xiaodaigh Jan 4, 2018

stevengj Jan 4, 2018

xiaodaigh commented Jan 4, 2018

xiaodaigh commented Jan 12, 2018

nalimilan commented Jan 12, 2018

xiaodaigh commented Jan 13, 2018

nalimilan commented Jan 13, 2018

xiaodaigh commented Jan 13, 2018

nalimilan commented Jan 13, 2018

xiaodaigh commented Jan 13, 2018

nalimilan commented Jan 13, 2018

xiaodaigh commented Jan 13, 2018

nalimilan commented Jan 14, 2018

nalimilan commented Jan 28, 2018

ViralBShah commented May 25, 2020

nalimilan Jul 9, 2020

nalimilan commented Sep 27, 2020

added string radix sort and sortperm + tests #27

added string radix sort and sortperm + tests #27

Conversation

xiaodaigh commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaodaigh commented Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaodaigh commented Jan 4, 2018

xiaodaigh commented Jan 12, 2018

nalimilan commented Jan 12, 2018

xiaodaigh commented Jan 13, 2018

nalimilan commented Jan 13, 2018

xiaodaigh commented Jan 13, 2018

nalimilan commented Jan 13, 2018

xiaodaigh commented Jan 13, 2018

nalimilan commented Jan 13, 2018

xiaodaigh commented Jan 13, 2018

nalimilan commented Jan 14, 2018

nalimilan commented Jan 28, 2018

ViralBShah commented May 25, 2020

Choose a reason for hiding this comment

nalimilan commented Sep 27, 2020

xiaodaigh commented Jan 4, 2018 •

edited

Loading