fixed logical error in parameter "hi" the bisect functions of the bin… - #13900
fixed logical error in parameter "hi" the bisect functions of the bin…#13900Nisarg-patel-2410 wants to merge 2 commits into
Conversation
algorithms-keeper
Bot
added
enhancement
for more information, see https://pre-commit.ci
| """ | ||
| if hi < 0: | ||
| hi = len(sorted_collection) | ||
| hi = len(sorted_collection) + hi + 1 # in case of negetive indexing used for hi |
There was a problem hiding this comment.
Good fix for negative indexing support. However there's a typo in
the inline comment — "negetive" should be "negative":
in case of negative indexing used for hi
| """ | ||
| if hi < 0: | ||
| hi = len(sorted_collection) | ||
| hi = len(sorted_collection) + hi + 1 # in case of negetive indexing used for hi |
There was a problem hiding this comment.
Worth adding a doctest that demonstrates the negative index behavior
to verify this fix works as expected and prevent regressions:
binary_search([1, 2, 3, 4, 5], 4, 0, -1)
3
This would make the edge case explicitly testable.
priya-sundaram-dev
left a comment
priya-sundaram-dev
left a comment
There was a problem hiding this comment.
Thanks for looking at this, but I think the new formula isn't quite right and needs a bit more work before it can go in.
The change makes hi < 0 mean "negative index from the end" via hi = len(sorted_collection) + hi + 1. Testing that against normal negative-index semantics, it's off by one: for sorted_collection = [0, 5, 7, 10, 15], hi = -1 yields hi = 5 (the whole array), whereas negative indexing would make -1 refer to the last element (hi = 4). So -1 and the old hi default now behave identically, and the intended "stop before the last element" case isn't reachable.
A few asks before this can be approved:
- Decide and document the exact semantics of a negative
hi(the docstring says this mirrorsbisect.bisect_left, and CPython'sbisectdoesn't special-case negativehiat all — worth confirming the desired behaviour). - Add doctests that exercise negative
hifor bothbisect_leftandbisect_right, since the current examples don't cover it and CI can't catch a regression here. - Minor: typo in the comment, "negetive" → "negative".
Happy to re-review once the semantics are pinned down and covered by a doctest. 🙏
cclauss
added
awaiting changes
4 participants
Describe your change:
This update improves how the binary search function handles negative values passed to hi. Earlier, when hi was negative, the code reset it to len(sorted_collection), which ignored Python’s negative-index behavior.
The new logic adjusts hi by converting the negative index into its proper positive position. This makes the function behave more consistently with Python slicing rules.
What Changed
Added support for negative indexing in the hi parameter:
if hi < 0: hi = len(sorted_collection) + hi + 1This ensures the search range is interpreted correctly when callers use negative indices.
Checklist: