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

sort! docstring does not document order argument #22382

Open
nalimilan opened this issue Jun 15, 2017 · 11 comments
Open

sort! docstring does not document order argument #22382

nalimilan opened this issue Jun 15, 2017 · 11 comments
Labels
docs This change adds or pertains to documentation good first issue Indicates a good issue for first-time contributors to Julia sorting Put things in order

Comments

@nalimilan
Copy link
Member

Title says it all. That is particularly confusing is that "order" is mentioned twice, but in reference to rev, not to order.

help?> sort!
search: sort! sortperm! sort sortrows sortperm sortcols Cshort issorted

  sort!(v; alg::Algorithm=defalg(v), lt=isless, by=identity, rev::Bool=false, order::Ordering=Forward)

  Sort the vector v in place. QuickSort is used by default for numeric arrays
  while MergeSort is used for other arrays. You can specify an algorithm to
  use via the alg keyword (see Sorting Algorithms for available algorithms).
  The by keyword lets you provide a function that will be applied to each
  element before comparison; the lt keyword allows providing a custom "less
  than" function; use rev=true to reverse the sorting order. These options are
  independent and can be used together in all possible combinations: if both
  by and lt are specified, the lt function is applied to the result of the by
  function; rev=true reverses whatever ordering specified via the by and lt
  keywords.

@nalimilan nalimilan added the docs This change adds or pertains to documentation label Jun 15, 2017
@dpsanders
Copy link
Contributor

dpsanders commented Jun 15, 2017

It seems to me that there are too many redundant options. Why is it not sufficient with just a single by argument, for example?

@nalimilan
Copy link
Member Author

I guess there are cases in which you cannot reverse the sorting order without the rev argument: for reals, you can always use by=x -> -x, but it doesn't work e.g. for dates.

@andreasnoack
Copy link
Member

But lt, rev, and order seem very overlapping.

julia> x = randn(4);

julia> sort(x, lt = (x,y) -> isless(y,x)) == sort(x, rev = true) == sort(x, order=Base.Order.Reverse)

true

julia> x = [(rand(1:2), rand(1:2)) for i in 1:5]
5-element Array{Tuple{Int64,Int64},1}:
 (1, 1)
 (1, 1)
 (2, 1)
 (2, 1)
 (2, 2)

julia> sort(x, lt = (x,y) -> isless(y,x)) == sort(x, rev = true) == sort(x, order=Base.Order.Reverse)

true

@dpsanders
Copy link
Contributor

@nalimilan Ah, yes, maybe I meant a single lt argument.

@nalimilan
Copy link
Member Author

I agree rev and order seem to overlap, though I don't know what the latter does since it's undocumented. OTOH passing lt = (x,y) -> isless(y,x) is quite verbose compared to rev=true.

@andreasnoack
Copy link
Member

OTOH passing lt = (x,y) -> isless(y,x) is quite verbose compared to rev=true.

In most cases, you can just do lt = > which is shorter than rev=true. The verbosity is because of Julia's total/patial order thing and because we don't define isgreater.

@StefanKarpinski
Copy link
Member

The order argument is kind of an implementation detail, so not documenting it was intentional. It will likely be removed in the next release. I suppose we could document it as a transient API in 0.6.

@nalimilan
Copy link
Member Author

The problem is that it appears in the docstring. FWIW, this wasn't the case in 0.6. Should we just remove it?

@StefanKarpinski
Copy link
Member

Your call.

@nalimilan
Copy link
Member Author

I've filed #22388, applying the most radical fix.

@StefanKarpinski
Copy link
Member

x-ref: #19295

@ViralBShah ViralBShah added the good first issue Indicates a good issue for first-time contributors to Julia label Mar 11, 2022
@LilithHafner LilithHafner 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
docs This change adds or pertains to documentation good first issue Indicates a good issue for first-time contributors to Julia sorting Put things in order
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants