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

Adding perm version of sortslices #45787

Closed
wants to merge 4 commits into from
Closed

Adding perm version of sortslices #45787

wants to merge 4 commits into from

Conversation

jgonzalez49
Copy link

I saw that #45211 mentioned #9258 was still open because we need a perm version of sortslices, so here is an attempt simply snipping out the assignment lines from the existing sortslices function and returning the vector it creates for how the sort will go down.

@mkitti
Copy link
Contributor

mkitti commented Jul 12, 2022

@oscardssmith or @LilithHafner could we review this as well to close 9258?

@gbaraldi
Copy link
Member

gbaraldi commented Jul 12, 2022

Maybe add some examples to the docstring? Similar to sortslices. Other than that LGTM

Mirrored documentation from sortslices into sortslicesperm.
Added jldoctest annotations to sortslices and sortslicesperm where they were missing.
@jgonzalez49
Copy link
Author

Thanks for the feedback, I have now mirrored the documentation for sortslices into the documentation for sortslicesperm. I also noticed that the sortslices higher dimensional examples were missing the jldoctest annotation so I added those in and fixed the sortslicesperm higher dimensional examples to match.

@mkitti
Copy link
Contributor

mkitti commented Jul 12, 2022

The doc string is getting long now. Consider making part of it an # Extended Help section that can be accessed with ??sortslicesperm.

See item 11 in the documentation here:
https://docs.julialang.org/en/v1/manual/documentation/

@LilithHafner
Copy link
Member

Thanks, @jgonzalez49! This looks like a pretty good implementation. I like that you primarily reused functionality from the existing sortslices function.

Unfortunately, this is a tricky API decision. Sorting has a lot of parameters, including

  • mutating vs non-mutating
  • sort vs sortperm
  • whether to do a partial sort
  • whether to sort slices
  • whether to sort reverse
  • comparison function
  • by function
  • which dimension to sort according to (for multidimensional arrays)
  • ...

We currently specify the first three with function names and the last four with keyword arguments. To specify n binary factors with function names takes 2^n names. I think that n = 3 may already be too much. I'd like to see the four partialsort* functions replaced with a single keyword partial in Julia 2.0.

If we look at the current list of exported sorting functions:

    partialsort,
    partialsort!,
    partialsortperm,
    partialsortperm!,
    sort!,
    sort,
    sortperm,
    sortperm!,
    sortslices,
    (sortslicesperm,)

sortslices is clearly the odd one out. We have a 2x2x2 cartesian product and then a stray. Adding sortslicesperm would begin to remedy the incompleteness of this situation, but to fully resolve it would require exporting an additional 7 methods, not just the 1 from this pr. I really don't want to export partialsortslicesperm!.

In this particular case, sortsliceperm ≈ sortperm ∘ eachslice, so I think sortperm ∘ eachslice should solve the linked issue now that eachslice infers well.

julia> @benchmark sortslicesperm(rand(100, 100), dims=1)
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min  max):  29.638 μs    6.823 ms  ┊ GC (min  max):  0.00%  98.31%
 Time  (median):     69.910 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   82.902 μs ± 258.888 μs  ┊ GC (mean ± σ):  13.17% ±  4.17%

                            ▃▇█▆▃▂                              
  ▂▃▃▃▂▂▂▂▂▁▁▂▁▂▂▂▂▂▂▁▂▁▂▃▅████████▆▆▅▅▄▄▄▃▃▃▃▃▃▃▃▂▃▂▂▂▂▂▂▂▂▂▂ ▃
  29.6 μs         Histogram: frequency by time          111 μs <

 Memory estimate: 85.12 KiB, allocs estimate: 12.

julia> @benchmark sortslicesperm(rand(100, 100), dims=2)
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min  max):  23.647 μs    5.879 ms  ┊ GC (min  max):  0.00%  98.45%
 Time  (median):     62.347 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   74.238 μs ± 228.614 μs  ┊ GC (mean ± σ):  12.89% ±  4.17%

                        ▅█▅▃                                    
  ▃▃▂▂▂▂▂▂▁▂▂▂▂▂▁▁▁▁▂▂▄▇█████▇▆▅▄▃▃▃▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂ ▃
  23.6 μs         Histogram: frequency by time          118 μs <

 Memory estimate: 85.12 KiB, allocs estimate: 12.

julia> @benchmark sortperm(eachslice(rand(100, 100), dims=1))
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min  max):  25.618 μs    9.014 ms  ┊ GC (min  max):  0.00%  96.37%
 Time  (median):     66.576 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   79.331 μs ± 260.467 μs  ┊ GC (mean ± σ):  13.33% ±  4.05%

                        ▄██▆▅▂▁                                 
  ▂▃▃▂▂▂▂▂▂▁▂▂▁▁▁▂▂▂▁▂▃▇███████▇▆▄▄▄▃▃▃▃▃▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂▂ ▃
  25.6 μs         Histogram: frequency by time          122 μs <

 Memory estimate: 79.11 KiB, allocs estimate: 6.

julia> @benchmark sortperm(eachslice(rand(100, 100), dims=2))
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min  max):  20.000 μs    6.657 ms  ┊ GC (min  max):  0.00%  98.74%
 Time  (median):     59.200 μs               ┊ GC (median):     0.00%
 Time  (mean ± σ):   71.013 μs ± 255.866 μs  ┊ GC (mean ± σ):  14.77% ±  4.07%

                                  ▄██▇▄                         
  ▃▃▂▂▂▂▂▂▁▁▁▁▂▂▁▂▁▁▁▂▁▁▁▁▁▁▁▂▁▂▄▇██████▆▇▆▆▅▄▃▃▃▃▃▃▃▃▃▂▂▂▂▂▂▂ ▃
  20 μs           Histogram: frequency by time         85.9 μs <

 Memory estimate: 79.11 KiB, allocs estimate: 6.

@jgonzalez49
Copy link
Author

Sounds good, glad the functionality is handled. I'll go ahead and close this PR.

@LilithHafner
Copy link
Member

Sorry that #9258 was misclassified.

@ViralBShah ViralBShah added the sorting Put things in order label Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants