fix(searches): correct ternary search pivots and exclusive bounds - #15005
Open
SEPURI-SAI-KRISHNA wants to merge 1 commit into
Open
fix(searches): correct ternary search pivots and exclusive bounds#15005SEPURI-SAI-KRISHNA wants to merge 1 commit into
SEPURI-SAI-KRISHNA wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your change:
ite_ternary_searchandrec_ternary_searchraiseIndexErroron most non-trivialinputs, and return
-1for elements that are present.There are two independent defects.
1. The pivots are not offsets into the current search window.
These are derived from
left + right, so they are not positions insidearray[left:right]. As soon asleftgrows, they run past the end of the array.For example with
left = 100, right = 115the second pivot evaluates to144,well outside the window and outside a 115-element array. Roughly 46% of searches
on arrays of 30-300 elements crash with
IndexError.2. The narrowing steps drop one element per step.
rightis an exclusive bound everywhere else in this file — it is initialised tolen(array), andlin_searchiteratesrange(left, right). But the narrowing usedright = one_third - 1andright = two_third - 1, which discards the element atthe new
rightindex. Since the pivot itself has already been compared and ruledout, only
right = one_third/right = two_thirdis correct. This silently losesa candidate on every iteration, so present values report as not found:
On arrays of 10-30 elements about 6.6% of lookups for a value that is present
return
-1.Why the existing doctests never caught this. Every current doctest uses an array
shorter than
precision = 10, soright - left < precisionis true immediately andthe function returns from
lin_searchon the first iteration. The ternary logic wasnever executed by any test.
Fix
Compute both pivots as offsets inside the half-open window
array[left:right], andnarrow with the exclusive bound the rest of the file already uses:
Both pivots are now guaranteed to satisfy
left <= one_third <= two_third < right,and every branch strictly shrinks the window, so the search always terminates.
The loop guard also becomes
while left < right, matching the half-open range (withleft == rightthe window is empty).Two further small corrections in the same file:
lin_search's docstring now states thatleftis inclusive andrightisexclusive, which is what the implementation has always done.
__main__block calledrec_ternary_search(0, len(collection) - 1, ...),which excluded the last element of the user's input, so searching for the largest
value printed "Not found". Corrected to
len(collection).No behaviour that the existing doctests rely on has changed — all of them still pass
unmodified.
Verification
20,000 randomised sorted arrays (lengths 0-400, duplicates included), each searched
for both a present and a possibly-absent value, against both the iterative and the
recursive variant: 0 failures. On
masterthe same run produces thousands ofIndexErrors and false-1s.ite_ternary_search(list(range(200)), 150)now returns150.Arrays with heavy duplicates return a valid index of the target; string arrays still
work.
Added doctests that exercise the ternary path itself (a 100-element list, searched
for every member plus one absent value) — these fail on
masterand pass here.ruff check,ruff format --check,mypy --ignore-missing-imports,pytest --doctest-modules searches/ternary_search.pyandpre-commit runall pass.Add an algorithm?
Fix a bug or typo in an existing algorithm?
Add or change doctests? -- Note: Please avoid changing both code and tests in a single pull request.
Documentation change?
Checklist: