Implement sortslices, deprecate sortrows/sortcols #28332

Keno · 2018-07-28T22:56:12Z

As discussed on triage, sortslices is the higher dimensional extension
of sortrows/sortcols. The dimensions being specified are the dimensions
(and for higher dimensions the order of the dimensions) to slice along. See
the help text for an example of the higher dimensional behavior. Deprecate
sortrows/sortcols in favor of sortslices.

ararslan · 2018-07-28T23:06:08Z

base/sort.jl

+               dims == :cols ? 2 :
+               error("Only :rows or :cols are allowed as symbolic dimensions")
+    end
+    _sortslices(A, Val{dims}(); kws...)


Looks like this file is included in the system image before Val is defined, which is causing the CI errors.

I'm actually not entirely convinced that it should live here. I think it might make more sense to go with multidimensional, esp since we now use this file from the compiler. This code has very little to do with sorting.

👍 That does seem like a more appropriate place for it, IMO.

ararslan · 2018-07-28T23:06:45Z

base/sort.jl

+Note that the default comparison function on one dimensional slices sorts
+lexicographically.
+
+For convenience, `dims=:rows` and `dims=:cols` are supported as alternative


We don't do this for anything else, why is this function special in that regard?

Fair enough, it was requested for convenience, but I think the behavior is consistent enough that we can do without.

ararslan · 2018-07-29T03:13:18Z

base/multidimensional.jl

+Note that the default comparison function on one dimensional slices sorts
+lexicographically.
+
+For convenience, `dims=:rows` and `dims=:cols` are supported as alternative


Can we get rid of these awkward special cases?

Agreed; this makes supporting named dimensions a breaking change, so we shouldn't have this now.

Keno · 2018-07-29T03:23:07Z

One thing to note for those looking at this, is that in some ways this is incompatible with mapslices, e.g.:

julia> mapslices(println, [1 2; 3 4]; dims=1)
[1, 3]
[2, 4]

julia> sortslices([1 2; 3 4]; by=println, dims=1)
[3, 4]
[1, 2]

I.e. the dims keyword has opposite meaning here.

JeffBezanson · 2018-07-29T06:46:27Z

There was some discussion of the meaning of the dims argument to mapslices in #18326.

JeffBezanson · 2018-07-29T06:49:13Z

base/multidimensional.jl

+# Higher dimensions
+
+`sortslices` extends naturally to higher dimensions. E.g., if `A` is a
+a 2x2x2 array, `sortslices(A, dims=2)` will slices along the 3rd dimension,


will slices => will slice
Did you mean dims=3 here?

JeffBezanson · 2018-07-29T06:53:08Z

base/multidimensional.jl

+
+If `dims` is a tuple, the order of the dimensions in `dims` is
+relevant and specifies the linear order of the slices. E.g., if `A` is three
+dimensional and `dims` is `(1,2)`, we will take slices in the dimension three,


extra the

mbauman · 2018-07-30T14:49:30Z

base/multidimensional.jl

+    sortslices(A; dims, alg::Algorithm=DEFAULT_UNSTABLE, lt=isless, by=identity, rev::Bool=false, order::Ordering=Forward)
+
+Sort slices of an array `A`. The keyword argument `dims` must be either
+an integer or a tuple of integers specifies along which dimension to take the


"take the slices" sounds like it's where the colon goes to me. I think a slightly better wording might be:

dims … specifies the dimension(s) over which the slices are sorted.

mbauman · 2018-07-30T14:52:50Z

base/multidimensional.jl

+# Higher dimensions
+
+`sortslices` extends naturally to higher dimensions. E.g., if `A` is a
+a 2x2x2 array, `sortslices(A, dims=3)` will slice along the 3rd dimension,


s/slice along/sort slices within/?

mbauman

Just minor doc comments

mbauman · 2018-07-30T15:13:22Z

With regards to the meaning of dims: one of the best arguments for this meaning is how an array with named dimensions might extend this method. There, you'd might want to name the first dimension "rows" and the second dimension "cols" (or something more meaningful), and then sortslices(A, dims=:rows) could be implemented to mean sortrows. Perhaps I should have been more clear that this was a thought experiment/extension in our initial discussion — sorry, Keno! (cf.
#28332 (comment))

I also think that this is aligned with the meaning of dims in sort itself. There, too, we're sorting "within" the named dimension — it's just individual elements instead of entire slices.

As discussed on triage, `sortslices` is the higher dimensional extension of `sortrows`/`sortcols`. The dimensions being specified are the dimensions (and for higher dimensions the order of the dimensions) to slice along. See the help text for an example of the higher dimensional behavior. Deprecate sortrows/sortcols in favor of sortslices.

Keno requested a review from mbauman July 28, 2018 22:56

ararslan added arrays [a, r, r, a, y, s] deprecation This change introduces or involves a deprecation labels Jul 28, 2018

ararslan reviewed Jul 28, 2018

View reviewed changes

Keno force-pushed the kf/sortslices branch from da5ba15 to b9639e7 Compare July 29, 2018 02:53

ararslan reviewed Jul 29, 2018

View reviewed changes

JeffBezanson reviewed Jul 29, 2018

View reviewed changes

Keno force-pushed the kf/sortslices branch from b9639e7 to 2987791 Compare July 29, 2018 22:47

ararslan mentioned this pull request Jul 30, 2018

Set VERSION to 0.7.0-rc1 #28294

Merged

13 tasks

mbauman reviewed Jul 30, 2018

View reviewed changes

JeffBezanson added this to the 0.7 milestone Jul 30, 2018

Keno force-pushed the kf/sortslices branch from 2987791 to 8463532 Compare July 30, 2018 19:39

mbauman approved these changes Jul 30, 2018

View reviewed changes

Keno merged commit 33e8b62 into master Jul 30, 2018

mbauman deleted the kf/sortslices branch July 30, 2018 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement sortslices, deprecate sortrows/sortcols #28332

Implement sortslices, deprecate sortrows/sortcols #28332

Keno commented Jul 28, 2018

ararslan Jul 28, 2018

Keno Jul 28, 2018

ararslan Jul 28, 2018

ararslan Jul 28, 2018

Keno Jul 28, 2018

ararslan Jul 29, 2018

JeffBezanson Jul 29, 2018

Keno commented Jul 29, 2018

JeffBezanson commented Jul 29, 2018

JeffBezanson Jul 29, 2018

Keno Jul 29, 2018

JeffBezanson Jul 29, 2018

mbauman Jul 30, 2018

mbauman Jul 30, 2018 •

edited

Loading

mbauman left a comment

mbauman commented Jul 30, 2018 •

edited

Loading

Implement sortslices, deprecate sortrows/sortcols #28332

Implement sortslices, deprecate sortrows/sortcols #28332

Conversation

Keno commented Jul 28, 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

Keno commented Jul 29, 2018

JeffBezanson commented Jul 29, 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

mbauman Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

mbauman left a comment

Choose a reason for hiding this comment

mbauman commented Jul 30, 2018 • edited Loading

mbauman Jul 30, 2018 •

edited

Loading

mbauman commented Jul 30, 2018 •

edited

Loading