Skip to content

Fix typo in heuristics of BracketedSort #52494

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

Merged
merged 2 commits into from
Dec 18, 2023
Merged

Fix typo in heuristics of BracketedSort #52494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9297113
fix typo in heuristics of BracketedSort
LilithHafner Dec 11, 2023
78aa8cf
Revert "accept that invalid lt continues to work"
LilithHafner Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion base/sort.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1262,7 +1262,7 @@ function _sort!(v::AbstractVector, a::BracketedSort, o::Ordering, kw)
k = cbrt(ln)
k2 = round(Int, k^2)
k2ln = k2/ln
offset = .15k2*top_set_bit(k2) # TODO for further optimization: tune this
offset = .15k*top_set_bit(k2) # TODO for further optimization: tune this
lo_signpost_i, hi_signpost_i =
(floor(Int, (tar - lo) * k2ln + lo + off) for (tar, off) in
((minimum(target), -offset), (maximum(target), offset)))
Expand Down
7 changes: 3 additions & 4 deletions test/sorting.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -721,10 +721,9 @@ end
for alg in safe_algs
@test sort(1:n, alg=alg, lt = (i,j) -> v[i]<=v[j]) == perm
end
# This could easily break with minor heuristic adjustments
# because partialsort is not even guaranteed to be stable:
@test partialsort(1:n, 172, lt = (i,j) -> v[i]<=v[j]) == perm[172]
@test partialsort(1:n, 315:415, lt = (i,j) -> v[i]<=v[j]) == perm[315:415]
# Broken by the introduction of BracketedSort in #52006 which is unstable
@test_broken partialsort(1:n, 172, lt = (i,j) -> v[i]<=v[j]) == perm[172]
Copy link
Member

@KristofferC KristofferC Dec 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this even worth having as a test if all we do is flip it between @test and @test_broken depending on the current state of the implementation?

Copy link
Member Author

@LilithHafner LilithHafner Dec 12, 2023

Choose a reason for hiding this comment

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

Yes. If I had listened to the test failure instead of blindly flipping it in #52006 then it would have caught the bug that this PR fixes.

This should not be a false positive for changes elsewhere in the codebase so the labor of switching it back and forth should be minimal.

Copy link
Member

@KristofferC KristofferC Dec 18, 2023

Choose a reason for hiding this comment

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

But changing a @test to a @test_broken means you have introduced a bug. But if the current code does not have a bug, then this test should not exist.

Copy link
Member Author

@LilithHafner LilithHafner Dec 18, 2023

Choose a reason for hiding this comment

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

This is a unit test of an internal to help identify unintended behavioral changes. It's okay to have tests that are not testing something that is a part of the public API. The only problem I can see with this test is the potential for false positives. However, so far it has had one true positive and zero false positives so I'm not worried about that yet.

@test_broken partialsort(1:n, 315:415, lt = (i,j) -> v[i]<=v[j]) == perm[315:415]

# lt can be very poorly behaved and sort will still permute its input in some way.
for alg in safe_algs
Expand Down