Fix typo in heuristics of BracketedSort #52494
Conversation
LilithHafner
added
performance
This reverts commit 069c453.
|
The test failures in the commit that introduced this typo should have alerted me that something as wrong. However, as they were of the form of |
| @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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The impact of this typo was a) massively decreased performance that was b) predicted by heuristic dispatch, resulting in this algorithm not being dispatched too. I introduced this typo in 5657e5f (a commit on PR #52006) after performing all the benchmarking.
Even with this typo there were no major performance regressions and there are no correction bugs, which makes it a hard thing to add tests for. I'm open to suggestions for how to test this.