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

Stop incorrectly documenting the default sorting algorithms #47303

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

LilithHafner
Copy link
Member

Notably, this is a strengthened claim. If this is change is released, then we are committed to always performing a stable sort by default (e.g. we cannot regress #42713).

(a small change with a big diff due to line width)

From #47297 (comment)

@LilithHafner LilithHafner added docs This change adds or pertains to documentation sorting Put things in order labels Oct 24, 2022
@jakobnissen
Copy link
Contributor

jakobnissen commented Oct 24, 2022

Two questions:

  • Is it breaking to stop promising the algorithm used, given that the precise algorithm was documented in the docstring?
  • If a user wants to opt-in to unstable sorting for increased speed (which may not be possible now, but quite likely could be possible in the future), it's not clear how to do that. Perhaps DEFAULT_UNSTABLE should be exported and documented, with no guarantees of the underlying algorithm other than it actually correctly sorts, such that users can easily opt-in to unstable sorting?

@LilithHafner
Copy link
Member Author

Is it breaking to stop promising the algorithm used, given that the precise algorithm was documented in the docstring?

Technically, yes. But the docstring was already a lie. In 1.8, we can see with @profview begin x = rand(1:3000, 10000); for _ in 1:10000 sort!(rand!(x, 1:3000)) end end (or @profile if not using VSCode) that sort!(::Vector{Int}) sometimes uses counting sort which is often faster than QuickSort in the cases when it is used, despite the fact that counting sort allocates.

I'd like to classify this loosening of the specific algorithm guarantee as a "minor change" as described by https://julialang.org/blog/2019/08/release-process/ as "technically breaking changes that are sufficiently unlikely to break anyone's code and which, in fact, do not break things in the package ecosystem as determined by running PkgEval to verify that there isn't any breakage."

We should probably run PkgEval on #45222 (or perhaps on the whole combination of 1.9 sorting changes) to support this classification.

If a user wants to opt-in to unstable sorting for increased speed (which may not be possible now, but quite likely could be possible in the future), it's not clear how to do that. Perhaps DEFAULT_UNSTABLE should be exported and documented, with no guarantees of the underlying algorithm other than it actually correctly sorts, such that users can easily opt-in to unstable sorting?

Yes. alg=Base.DEFAULT_UNSTABLE is currently the best (only) way to opt into potential future unstable optimizations. However, I'd like to consider changing that to stable=false in the future which I think is a nicer approach for when folks are willing to sacrifice stability but not interested in delving into specific algorithms and should therefore not set the alg keyword.

* Force stable algorithm

* Can be simplify by removing specific options

* Unify DEFAULT_STABLE usage instead of DEFAULT_UNSTABLE or defalg()

* Remove trailing whitespaces

* Do not combine specify/specific

* Revert `defalg`
@KristofferC
Copy link
Member

Is this good to go?

@LilithHafner
Copy link
Member Author

Yes.

@KristofferC KristofferC merged commit 5d6d830 into master Nov 2, 2022
@KristofferC KristofferC deleted the LilithHafner-patch-5 branch November 2, 2022 13:17
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 sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants