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

Improve documentation of sort #27661

Closed
bkamins opened this issue Jun 19, 2018 · 7 comments · Fixed by #48387
Closed

Improve documentation of sort #27661

bkamins opened this issue Jun 19, 2018 · 7 comments · Fixed by #48387
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

@bkamins
Copy link
Member

bkamins commented Jun 19, 2018

Following issue #26741 I have another problem with sort when using order argument. Consider the following inconsistency as an example (similar thing is present with lt):

julia> sort([1,-1,2], by=identity, order=Base.By(abs))
3-element Array{Int64,1}:
  1
 -1
  2

julia> sort([1,-1,2], by=x->-x, order=Base.By(abs))
3-element Array{Int64,1}:
  2
  1
 -1

In the first case, following the implementation in ordering.jl, Julia ignores by argument in the first case, because it is equal to the default and in the second case it respects it.

In general order argument is not documented well and it defaults to a non-exported value.
I am not sure, given we are in 0.7.0-alpha stage if this should be:

  • fixed (e.g. by not exposing order argument in public API)
    or
  • only documented precisely describing the way it works (or at least saying to either use lt, by and rev or order keyword argument)?
@haampie
Copy link
Contributor

haampie commented Jun 19, 2018

See also #22388 (comment) and the corresponding issue #22382.


Edit: the following digresses a bit form the point of this issue, but still.

I think we're pretty close to resolving the whole sort API problem altogether. There's @nalimilan's PR #22388 that deprecates the order keyword, and there's some work haampie@b65740d I have that radically implements the ideas of #26741 everywhere. There's only a decision to be made about what the API really should look like.

My question would be: can we make some minimal changes right now, so that we can resolve the full issue (order keyword not making sense, rev = true/false being type unstable, Order not being composable) during Julia 1.0 in a non-breaking way?

@haampie
Copy link
Contributor

haampie commented Jun 20, 2018

Crazy idea: for the whole zoo of sort-related functions deprecate the signature

sort(x, by = b, lt = l, rev = r)

to a positional argument

sort(x, ord(by = b, lt = l, rev = r))

Deprecate

sort(x, order = o)

to

sort(x, o)

This avoids (1) the type instability within the sort function -- only ord is type unstable now, and (2) it avoids the incompatibility between the kwargs. Then advocate the use of Order instances in most cases. Export some sensible defaults:

sort(x, rev = true)

becomes

# Backward = Rev(Forward), Forward = Lt(isless)
sort(x, Backward)

And make Order objects fully compositional. The following type-unstable call

sort(x, lt = my_op, rev = true, by = abs)

can be written in a stable way as

sort(x, Rev(By(abs, Lt(my_op))))

@bkamins
Copy link
Member Author

bkamins commented Jun 20, 2018

Given the number of open issues for sort API its consistency should be improved in my opinion. The question to core-devs is if it would be allowed at this stage.

@StefanKarpinski
Copy link
Member

Good thoughts but not gonna happen in 0.7/1.0; we're trying to get 0.7-beta out quite soon.

@vtjnash vtjnash added the docs This change adds or pertains to documentation label Apr 13, 2021
@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
@LilithHafner
Copy link
Member

The observed inconsistency in the OP is due to the fact that the by is applied in addition to, not instead of, the order keyword. That is, sort(v, by=f, order=By(g, Forward)) is equivalent to sort(v, by=g∘f).

@bkamins
Copy link
Member Author

bkamins commented Dec 3, 2022

My key point was, as noted in the title, that I do not find a clear documentation how different arguments interact (but maybe I missed something in the manual).

@LilithHafner
Copy link
Member

My comment was to help whomever documents this behavior not to invalidate the issue

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