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

Document the role of missing in PartialQuickSort and deprecate PartialQuickSort(::Integer) #47297

Closed

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner added docs This change adds or pertains to documentation deprecation This change introduces or involves a deprecation sorting Put things in order labels Oct 23, 2022
base/sort.jl Outdated
PartialQuickSort(k::OrdinalRange) = PartialQuickSort(first(k), last(k))
@deprecate PartialQuickSort(k::Integer) PartialQuickSort(missing, k)
Copy link
Member

@petvana petvana Oct 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't use @deprecate before, but it seems it should be placed into base/deprecated.jl.

@LilithHafner
Copy link
Member Author

I also haven't used deprecations in base before, and I'd like approval from someone who is confident that this is acceptable w.r.t. SymVer and our promise of backward compatibility.

@petvana
Copy link
Member

petvana commented Oct 23, 2022

As CI complains, the docs should be updated after merging your amazing stable QuickSort algorithm in #45222

https://github.com/JuliaLang/julia/blame/master/doc/src/base/sort.md#L143-L190

Just a few comments for sort.md

  • QuickSort is stable
  • QuickSort is not guaranteed to be O(n log n), strictly speaking
  • PartialQuickSort(k) needs to be removed
  • defalg(v::AbstractArray) = MergeSort is outdated

Further for sort.jl

  • MergeSort is never default alg.

    julia/base/sort.jl

    Lines 889 to 893 in 54e6899

    """
    sort!(v; alg::Algorithm=defalg(v), lt=isless, by=identity, rev::Bool=false, order::Ordering=Forward)
    Sort the vector `v` in place. [`QuickSort`](@ref) is used by default for numeric arrays while
    [`MergeSort`](@ref) is used for other arrays. You can specify an algorithm to use via the `alg`

Since I'm writing, I cannot find the stabilization of QuickSort in NEWS.md, and I think it's worthy.

More general question. Is there a plan to guarantee stability of sort(x) in v1.9 on? (And for sortpertm!-like functions?) The stability is now guaranteed (by documentation) only for QuickSort and AdaptiveSort. Thus the only way to rely on the stability is to use sort(x, alg=QuickSort) or sort(x, alg=AdaptiveSort), right?

I can try preparing some PRs, if you want.

@LilithHafner
Copy link
Member Author

Thank you for your observations! Yes, sort.md needs updating.

I can try preparing some PRs, if you want.

That would be lovely! Thank you! I think the PartialQuickSort entry should be changed, but not removed—it is still a valuable algorithm we have to offer, now even more so than before due to added performance in ranges that do not include the beginning of the vector. Also, the whole defalg system now seems a bit obsolete so the current state may still be in flux. Nevertheless, it is still clearly wrong as is, so a change would be warranted.

I happened to make a PR for the sort.jl problem you noted before I realized you were offering to make PRs, I'd be happy if you took as many of the rest as you'd like to :)

@LilithHafner
Copy link
Member Author

This PR deprecates improper usage of PartialQuickSort that is still found in sort.jl, so tests will fail until #47191 which fixes those usages is merged.

@LilithHafner
Copy link
Member Author

CI now passes (failures unrelated) and all this needs is approval from someone familiar with adding deprecations to Base (@vtjnash, perhaps)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation docs This change adds or pertains to documentation sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants