Skip to content

Add boundary checking to optimal bst - #11771

Merged
cclauss merged 3 commits into
TheAlgorithms:masterfrom
ivnvxd:optimize-optimal-bst
Sep 23, 2026
Merged

cclauss merged 3 commits into
TheAlgorithms:masterfrom
ivnvxd:optimize-optimal-bst

Conversation

@ivnvxd

@ivnvxd ivnvxd commented Oct 5, 2024

Copy link
Copy Markdown
Contributor

Describe your change:

The changes address potential issues with small intervals and overlapping boundaries in the current implementation of Knuth's optimization of Optimal BST algorithm.

  • Improved boundary checking in the find_optimal_binary_search_tree function
  • Added error handling for potential empty ranges with fallback to full range
  • Updated main loop to use new boundaries

Fixes #11724

  • 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:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms include at least one URL that points to Wikipedia or another similar explanation.
  • If this pull request resolves one or more open issues then the description above includes the issue number(s) with a closing keyword: "Fixes #ISSUE-NUMBER".

@algorithms-keeper algorithms-keeper Bot added enhancement This PR modified some existing files awaiting reviews This PR is ready to be reviewed labels Oct 5, 2024

@manuelaidos123 manuelaidos123 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tryt to Simplify the boundary checks for r_start and r_end, by adding additional checks to prevent IndexError, and replacing print statements with a logging mechanism, and configured logging for better debugging.

@ivnvxd

ivnvxd commented Oct 17, 2024

Copy link
Copy Markdown
Contributor Author

@manuelaidos123 Simplified boundary checks in Knuth's optimization for Optimal BST. Replaced the explicit fallback logic with direct min/max operations to ensure valid array bounds.
I think it would be better to make the logging configuration in a separate PR.

@priya-sundaram-dev priya-sundaram-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I applied the branch and the doctests still pass (5/5), so this is safe as a defensive change. My question before approving is whether it fixes an observed bug or is purely precautionary.

Knuth's optimization relies on the monotonicity invariant root[i][j-1] <= root[i][j] <= root[i+1][j], which (given the bottom-up DP fill order here) already keeps the temporal root r inside [i, j]. So under correct execution the extra max(i, min(...)) / min(j, max(...)) clamps should never actually clip anything, and the r > i / r < j guards are equivalent to the old r != i / r != j.

If you hit a concrete input where the old code went out of bounds or produced a wrong cost, could you add it as a doctest? That would (a) justify the change and (b) guard against regressions. If it is purely defensive with no failing case, a one-line comment saying so — plus dropping the now-redundant r_start/r_end if j > i else ... branches, since the loop body is already skipped when i == j — would keep the hot loop lean. Happy to approve either way once the intent is documented.

@cclauss cclauss added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels Sep 23, 2026
@cclauss

cclauss commented Sep 23, 2026

Copy link
Copy Markdown
Member

@priya-sundaram-dev Can you please add "solution" comments to this PR so we can get it reviewed and merged?

@priya-sundaram-dev

Copy link
Copy Markdown
Contributor

I reviewed the change against dynamic_programming/optimal_binary_search_tree.py and it looks correct and safe to merge:

  • The boundary clamping is sound. Knuth's optimization guarantees root[i][j-1] <= root[i][j] <= root[i+1][j], so the original range endpoints are already valid in the normal path; the added max(i, min(r_start, j)) / min(j, max(r_end, i)) clamps are purely defensive and cannot change the result on well-formed input. The j > i / i < j guards correctly handle the diagonal base case.
  • r != i/r != j → r > i/r < j is equivalent given r ∈ [i, j], and reads better.
  • The comment fix sum[i][j] → total[i][j] is a good catch — it now matches the actual variable name total.
  • Existing doctests still pass locally (python3 -m doctest -v: 5 passed, 0 failed).

On the "solution comments" request: the function is already reasonably documented, but the one thing that would make this fully self-explanatory for a reviewer is a one-line comment above the new clamp block stating why it exists — e.g. # Clamp the Knuth search window to [i, j] to guard against out-of-range roots on degenerate sub-problems. @ivnvxd, if you add that (it's your branch), I think this is merge-ready. LGTM otherwise.

@algorithms-keeper algorithms-keeper Bot removed the awaiting changes A maintainer has requested changes to this PR label Sep 23, 2026
@cclauss
cclauss merged commit 4e817a8 into TheAlgorithms:master Sep 23, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement This PR modified some existing files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimize and Test Optimal Binary Search Tree Implementation for optimal_binary_search_tree.py

4 participants