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.

Sign up for GitHub

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

Jump to bottom

Doc: The default sorting alg. is stable from 1.9 #47579

Merged
merged 16 commits into from Nov 21, 2022
Merged

Doc: The default sorting alg. is stable from 1.9 #47579

merged 16 commits into from Nov 21, 2022

Conversation

petvana
Copy link
Member

@petvana petvana commented Nov 15, 2022

This PR updates the docs after #45222 and resolves my comments from #47297 (comment)

Notice PartialQuickSort(::Integer) is about to be deprecated in #47297, and thus the documentation is removed as well.

Cc: @LilithHafner

Also relates to #47304

@petvana petvana added domain:docs This change adds or pertains to documentation domain:sorting Put things in order labels Nov 15, 2022
@PallHaraldsson
Copy link
Contributor

@LilithHafner Thanks for your work on sorting!

O(n²) is worst-case complexity, but it gets vanishingly unlikely

I know that, not worried, still might introsort variant be better; it avoids that? Might it be faster (with or without randomization)?

@petvana
Copy link
Member Author

petvana commented Nov 15, 2022

@PallHaraldsson Related discussion is here #45222 (comment)

@petvana
Copy link
Member Author

petvana commented Nov 18, 2022

@LilithHafner Can you please check the wording because it would be nice to merge this docs (and backport it) before 1.9 is out.

@petvana petvana added the backport 1.9 Change should be backported to release-1.9 label Nov 18, 2022
LilithHafner
LilithHafner reviewed Nov 18, 2022
View reviewed changes
Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

Sorry, thanks for the bump! I had this review lying around but hadn't submitted it.

doc/src/base/sort.md Show resolved Hide resolved
doc/src/base/sort.md Outdated Show resolved Hide resolved
doc/src/base/sort.md Outdated Show resolved Hide resolved
doc/src/base/sort.md Outdated Show resolved Hide resolved
doc/src/base/sort.md Outdated Show resolved Hide resolved
doc/src/base/sort.md Outdated Show resolved Hide resolved
doc/src/base/sort.md Outdated Show resolved Hide resolved
@petvana
Copy link
Member Author

petvana commented Nov 18, 2022 •
edited

@LilithHafner Thank you for the comments. I've resolved them + I've removed comments about v1.9 (7a66f76) as it is already noted in compat.

Therefore, it seems ready from my side.

LilithHafner
LilithHafner approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

I fixed a couple of typos and tweaked some wording. If it still looks good to you, I'm ready to merge.

doc/src/base/sort.md
map(x->issorted(x[k2]), (s, ps, qs)) # => (true, false, true)
s[1:k] == ps[1:k] # => true
s[k2] == qs[k2] # => true
```jldoctest
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for making this a doctest!

doc/src/base/sort.md Outdated Show resolved Hide resolved
doc/src/base/sort.md Outdated Show resolved Hide resolved
doc/src/base/sort.md Outdated Show resolved Hide resolved
doc/src/base/sort.md Outdated Show resolved Hide resolved
@petvana
Copy link
Member Author

petvana commented Nov 21, 2022

@LilithHafner Thanks for the changes. I've removed *appear* to be stable you reverted because it seems to me to be slightly misleading, since the default algorithm is always stable now (unless some library changes this behavior). Otherwise, it can be merged.

@LilithHafner
Copy link
Member

I've removed *appear* to be stable you reverted because it seems to me to be slightly misleading,

Agreed.

"" > "appear to be stable" > "appear to be fast"

@LilithHafner LilithHafner added the status:merge me PR is reviewed. Merge when all tests are passing label Nov 21, 2022
@LilithHafner LilithHafner merged commit c5fe17b into JuliaLang:master Nov 21, 2022
@LilithHafner LilithHafner removed the status:merge me PR is reviewed. Merge when all tests are passing label Nov 21, 2022
@LilithHafner
Copy link
Member

Thanks!

KristofferC pushed a commit that referenced this pull request Nov 28, 2022
@petvana @KristofferC
Doc: The default sorting alg. is stable from 1.9 (#47579)
4b4521f 
* Update doc/src/base/sort.md

* Update docs: The default sorting alg. is stable

* Compat 1.9 for QuickSort to be stable

* Specify the default algorithm

* Use example from InlineStrings.jl

* Change example to jldoctest

* Remove "*appear* to be stable." as slightly misleading.

Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
(cherry picked from commit c5fe17b)
KristofferC pushed a commit that referenced this pull request Nov 28, 2022
@petvana @KristofferC
Doc: The default sorting alg. is stable from 1.9 (#47579)
b5d6b03 
* Update doc/src/base/sort.md

* Update docs: The default sorting alg. is stable

* Compat 1.9 for QuickSort to be stable

* Specify the default algorithm

* Use example from InlineStrings.jl

* Change example to jldoctest

* Remove "*appear* to be stable." as slightly misleading.

Co-authored-by: Lilith Orion Hafner <lilithhafner@gmail.com>
(cherry picked from commit c5fe17b)
@KristofferC KristofferC mentioned this pull request Nov 28, 2022
Merged
51 tasks
@LilithHafner LilithHafner mentioned this pull request Dec 4, 2022
Closed
@KristofferC KristofferC mentioned this pull request Dec 14, 2022
Merged
26 tasks
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LilithHafner LilithHafner LilithHafner approved these changes

Assignees
No one assigned
Labels
domain:docs This change adds or pertains to documentation domain:sorting Put things in order
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@petvana @PallHaraldsson @LilithHafner @KristofferC