Lowest common ancestor of binary tree nodes #111
Conversation
BrianLusina added enhancement Algorithm 
| Warning Rate limit exceeded@BrianLusina has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughAdds parent-pointer support, equality/hash on tree nodes, two LCA utilities with tests and a README; plus numerous cosmetic formatting edits, minor refactors, and updated project metadata and dev tooling in pyproject.toml and DIRECTORY.md. Changes
Sequence Diagram(s)sequenceDiagram participant N1 as node_one participant N2 as node_two participant S1 as set_ancestors participant P1 as ptr1 participant P2 as ptr2 participant L as LCA rect rgb(245,250,245) note left of S1: Ancestor-set approach N1->>S1: climb parents, add to set N2->>S1: climb parents, check membership alt found S1->>L: return first matching ancestor else S1->>L: return None end end rect rgb(235,245,255) note left of P1: Two-pointer pointer approach N1->>P1: start ptr1 N2->>P2: start ptr2 loop until meet P1->>P1: move to parent or switch to N2 when None P2->>P2: move to parent or switch to N1 when None alt equal P1->>L: meeting node (LCA) end end end Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
datastructures/trees/binary/node.py (1)
164-176: Incorrect return type annotation.The return type annotation
List["BinaryTreeNode"] | Noneis incorrect. The method implementation always returns a list (lines 169-176), neverNone. An empty list[]is returned when there are no children.Apply this diff to correct the type annotation:
@property - def children(self) -> List["BinaryTreeNode"] | None: + def children(self) -> List["BinaryTreeNode"]: """Returns children of this node. Returns: List: children of this node in a list
🧹 Nitpick comments (3)
DIRECTORY.md (1)
287-293: Fix Markdown list indentation to satisfy markdownlint (MD007)The new
Test UtilsandUtilsbullets (and theTreesubtree they sit beside) are over‑indented relative to their parentBinaryitem, which triggers MD007 (“unordered list indentation”) on lines 287–293.Consider reducing the indentation by two spaces for this block so it matches the surrounding
Node/Search Tree/Treebullets, e.g.:- * [Test Utils](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/test_utils.py) - * Tree - * [Test Binary Tree](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree.py) - * [Test Binary Tree Deserialize](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_deserialize.py) - * [Test Binary Tree Serialize](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_serialize.py) - * [Test Binary Tree Visible Nodes](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_visible_nodes.py) - * [Utils](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/utils.py) + * [Test Utils](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/test_utils.py) + * Tree + * [Test Binary Tree](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree.py) + * [Test Binary Tree Deserialize](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_deserialize.py) + * [Test Binary Tree Serialize](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_serialize.py) + * [Test Binary Tree Visible Nodes](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/tree/test_binary_tree_visible_nodes.py) + * [Utils](https://github.com/BrianLusina/PythonSnips/blob/master/datastructures/trees/binary/utils.py)Adjust exact spacing to align with your preferred list style, but keeping a consistent 2‑space per level indent will silence MD007.
pystrings/similar_string_groups/__init__.py (1)
55-66:are_similar/num_similar_groups_2logic matches the original, but consider type hints + a cheap length guardThe new implementation correctly mirrors the original similarity definition:
are_similarallows only 0 diffs (identical strings) or 2 diffs where the differing pairs are reverses, i.e., exactly one swap — same semantics asis_similar.num_similar_groups_2usesUnionFindwith pairwise unions onare_similar, and the root‑set size matchesDisjointSetUnion.get_count()behavior.Two small polish suggestions:
- Add explicit type hints to
are_similarto stay consistent with the rest of the module:-# Helper: Decide if two strings are similar -def are_similar(s1, s2): +# Helper: Decide if two strings are similar +def are_similar(s1: str, s2: str) -> bool:
- Optionally harden against mismatched lengths (even if problem constraints guarantee equal length):
def are_similar(s1: str, s2: str) -> bool: - diff = [] + if len(s1) != len(s2): + return False + + diff = []You could also consider reusing this helper inside
num_similar_groups(instead of its nestedis_similar) to keep all similarity logic in one place, but that’s more of a code‑deduplication nicety than a requirement.Also applies to: 68-81
datastructures/trees/binary/utils.py (1)
4-42: Misleading terminology in docstring.The docstring on line 12 mentions "performs a depth-first search from node_two" but the implementation simply traverses upward via parent pointers, not a depth-first search. Consider revising to "traverses upward from node_two" for clarity.
Apply this diff to improve the docstring accuracy:
- Then, it performs a depth-first search from node_two, checking each ancestor. The first node it finds that's already + Then, it traverses upward from node_two via parent pointers, checking each ancestor. The first node it finds that's alreadyAdditionally, consider adding input validation to handle None inputs:
def lowest_common_ancestor( node_one: BinaryTreeNode, node_two: BinaryTreeNode ) -> BinaryTreeNode | None: """ Returns the lowest common ancestor of 2 nodes in the Binary Tree. ... """ + if node_one is None or node_two is None: + return None + # create a set to store all ancestors of node_one...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
datastructures/trees/binary/images/examples/lowest_common_ancestor_example_1.pngis excluded by!**/*.pngdatastructures/trees/binary/images/examples/lowest_common_ancestor_example_2.pngis excluded by!**/*.pngdatastructures/trees/binary/images/examples/lowest_common_ancestor_example_3.pngis excluded by!**/*.pngdatastructures/trees/binary/images/examples/lowest_common_ancestor_example_4.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/lowest_common_ancestor_solution_1.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/lowest_common_ancestor_solution_2.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/lowest_common_ancestor_solution_3.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/lowest_common_ancestor_solution_4.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/lowest_common_ancestor_solution_5.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/lowest_common_ancestor_solution_6.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/lowest_common_ancestor_solution_7.pngis excluded by!**/*.png
📒 Files selected for processing (22)
DIRECTORY.md(1 hunks)algorithms/arrays/two_sum_less_k/test_two_sum.py(2 hunks)algorithms/search/binary_search/maxruntime_n_computers/__init__.py(1 hunks)algorithms/search/binary_search/maxruntime_n_computers/test_max_runtime.py(1 hunks)datastructures/__init__.py(1 hunks)datastructures/sets/union_find/__init__.py(2 hunks)datastructures/streams/stream_checker/__init__.py(0 hunks)datastructures/streams/stream_checker/test_stream_checker.py(1 hunks)datastructures/trees/binary/README.md(1 hunks)datastructures/trees/binary/node.py(4 hunks)datastructures/trees/binary/search_tree/__init__.py(1 hunks)datastructures/trees/binary/test_utils.py(1 hunks)datastructures/trees/binary/tree/__init__.py(1 hunks)datastructures/trees/binary/utils.py(1 hunks)datastructures/trees/node.py(1 hunks)datastructures/trees/trie/__init__.py(1 hunks)puzzles/heap/maximal_score_after_k_operations/test_maximal_score.py(1 hunks)pystrings/is_prefix/test_is_prefix_of_word.py(1 hunks)pystrings/longest_self_contained_substring/__init__.py(1 hunks)pystrings/longest_self_contained_substring/test_longest_self_contained_substring.py(1 hunks)pystrings/similar_string_groups/__init__.py(2 hunks)pystrings/similar_string_groups/test_similar_string_groups.py(2 hunks)
💤 Files with no reviewable changes (1)
- datastructures/streams/stream_checker/init.py
🧰 Additional context used
🧬 Code graph analysis (6)
datastructures/trees/binary/utils.py (1)
datastructures/trees/binary/node.py (1)
BinaryTreeNode(6-202)
datastructures/trees/binary/test_utils.py (2)
datastructures/trees/binary/utils.py (2)
lowest_common_ancestor(4-42)lowest_common_ancestor_ptr(45-54)datastructures/trees/binary/node.py (1)
BinaryTreeNode(6-202)
puzzles/heap/maximal_score_after_k_operations/test_maximal_score.py (1)
puzzles/heap/maximal_score_after_k_operations/__init__.py (1)
max_score(6-26)
pystrings/similar_string_groups/test_similar_string_groups.py (1)
pystrings/similar_string_groups/__init__.py (1)
num_similar_groups_2(68-81)
datastructures/trees/binary/node.py (1)
datastructures/trees/ternary/node.py (1)
children(25-26)
algorithms/arrays/two_sum_less_k/test_two_sum.py (1)
algorithms/arrays/two_sum_less_k/__init__.py (1)
two_sum_less_than_k(4-27)
🪛 markdownlint-cli2 (0.18.1)
DIRECTORY.md
287-287: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
288-288: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
289-289: Unordered list indentation
Expected: 6; Actual: 8
(MD007, ul-indent)
290-290: Unordered list indentation
Expected: 6; Actual: 8
(MD007, ul-indent)
291-291: Unordered list indentation
Expected: 6; Actual: 8
(MD007, ul-indent)
292-292: Unordered list indentation
Expected: 6; Actual: 8
(MD007, ul-indent)
293-293: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
🔇 Additional comments (27)
datastructures/sets/union_find/__init__.py (1)
48-74: Formatting changes approved.The modifications are purely whitespace and newline adjustments (line 50: blank line after class docstring; line 74: spacing around return statement) for consistency with the codebase style. No functional changes to the Union-Find implementations.
datastructures/streams/stream_checker/test_stream_checker.py (1)
34-35: Standardized main guard quotesSwitching to double quotes here is purely stylistic; test execution behavior is unchanged and consistent with the rest of the repo.
datastructures/__init__.py (1)
3-3: all reformat is safeCondensing
__all__to a single line keeps the same exports and has no behavioral impact.algorithms/arrays/two_sum_less_k/test_two_sum.py (1)
8-8: List literal spacing change onlyAdded spaces after commas in test input lists; test semantics and coverage are unchanged, readability is slightly improved.
Also applies to: 24-24, 32-32, 40-40
puzzles/heap/maximal_score_after_k_operations/test_maximal_score.py (1)
7-7: Formatting-only updates to test data and main guardList spacing and the
__main__guard quote style are now consistent with the rest of the project; all test inputs, expected values, and control flow remain the same.Also applies to: 14-14, 21-21, 28-28, 35-35, 42-42, 49-49, 56-56, 63-170, 177-177
pystrings/longest_self_contained_substring/__init__.py (1)
48-48: Slice spacing change is behaviorally identicalThe slice
s[start : end + 1]is equivalent to the previous form; this is a cosmetic change aligning with formatter style, with no impact on results.pystrings/longest_self_contained_substring/test_longest_self_contained_substring.py (1)
141-142: Consistent main guard quotingUpdating the
__name__comparison to use double quotes is a stylistic normalization and does not affect test execution.algorithms/search/binary_search/maxruntime_n_computers/__init__.py (1)
64-72: Binary-search update is purely cosmetic, logic remains soundThe
max_run_time_2loop still implements the correct upper-bound binary search for the maximum feasible time; the assignments toleft/rightare unchanged semantically. No further action needed.pystrings/is_prefix/test_is_prefix_of_word.py (1)
56-57: Main-guard quote normalization is fineSwitching to double quotes in the
__main__guard is stylistic only and keeps behavior identical.algorithms/search/binary_search/maxruntime_n_computers/test_max_runtime.py (1)
7-40: Reformatting of parameterized tests preserves all casesThe expanded list formatting and quote-style change are cosmetic; the tuples
(batteries, n, expected)are unchanged, so coverage and assertions remain identical.datastructures/trees/trie/__init__.py (1)
5-5: Compact__all__declaration is fineCollapsing
__all__to a single line keeps the exported names identical while simplifying the module header.pystrings/similar_string_groups/test_similar_string_groups.py (1)
24-36: Multi-line test inputs and main-guard change are safeReformatting the long
strslists across lines and normalizing the__main__guard to double quotes do not alter the test data or expectations; tests still exercise both implementations with identical cases.Also applies to: 58-69, 71-72
datastructures/trees/binary/tree/__init__.py (1)
648-655: Stack initialization reformatting does not change behaviorSplitting the initial
stacklist into multiple lines maintains the same(node, length, last_direction)tuple and preserves the iterative zig-zag logic; this is a readability-only change.datastructures/trees/binary/search_tree/__init__.py (1)
16-16: LGTM! Formatting improvement.The type annotation quote style has been standardized to use double quotes, aligning with the broader formatting updates across the codebase.
datastructures/trees/binary/test_utils.py (6)
7-26: LGTM! Well-structured test case.The test correctly constructs a binary tree with parent pointers and validates the LCA of two sibling nodes. The parent pointers are properly initialized throughout the tree structure.
28-44: LGTM! Cross-subtree LCA test is correct.This test validates the LCA when nodes are in different subtrees, correctly expecting the root as their common ancestor.
46-65: LGTM! Ancestor-descendant test case is correct.This test properly validates the edge case where one node is an ancestor of the other. The LCA correctly returns the ancestor node itself, which aligns with the problem definition.
67-84: LGTM! Additional cross-subtree validation.This test provides additional coverage for the cross-subtree LCA scenario with a different tree structure.
87-165: LGTM! Comprehensive test coverage for pointer-based LCA.The test class provides thorough validation of the
lowest_common_ancestor_ptrfunction with test cases mirroring the first test class, ensuring both LCA implementations handle the same scenarios correctly.
168-169: LGTM! Standard test runner.The unittest.main() call enables the module to be run as a standalone test script.
datastructures/trees/node.py (3)
15-32: LGTM! Parent pointer support added correctly.The constructor properly adds the parent parameter with appropriate type hints and comprehensive documentation. The implementation correctly initializes the parent reference.
37-53: LGTM! Equality comparison properly implemented.The
__eq__method correctly handles None checks, type validation, and compares nodes based on their data values. This data-based equality is appropriate for the LCA use case where node identity is determined by the data they contain.
55-56: LGTM! Hash implementation maintains equality contract.The
__hash__method correctly bases the hash on the node's data, which is consistent with the__eq__implementation. This allows nodes to be used in hash-based collections like sets and dictionaries, which is essential for the LCA algorithm indatastructures/trees/binary/utils.py.datastructures/trees/binary/node.py (3)
8-10: LGTM! Docstring updated to reflect parent pointer support.The class docstring accurately documents the parent pointer capability, which enables upward traversal in the tree structure.
13-35: LGTM! Parent parameter properly integrated.The constructor correctly adds the parent parameter and passes it to the base class. The docstring comprehensively documents all parameters.
Note: When providing
leftorrightchild nodes in the constructor, their parent pointers must be set manually by the caller, as demonstrated in the test file (datastructures/trees/binary/test_utils.py).
201-202: LGTM! Hash method enables set membership.The
__hash__method correctly hashes based on the node's data, maintaining consistency with the__eq__method (lines 186-199). This enables nodes to be stored in the ancestor set used by the LCA algorithm indatastructures/trees/binary/utils.py.datastructures/trees/binary/README.md (1)
1-67: All referenced image files exist in the repository.Verification confirms that all 4 example images and 7 solution images referenced in the README are present in the expected directories (
images/examples/andimages/solutions/). No action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
puzzles/poker/__init__.py (1)
22-23: Ace‑low straight detection is broken due to tuple vs list comparison
ranksis a tuple fromzip(*groups), soif ranks == [14, 5, 4, 3, 2]:will never be true. This means A‑5 straights are never normalized to[5, 4, 3, 2, 1].- counts, ranks = zip(*groups) - if ranks == [14, 5, 4, 3, 2]: - ranks = [5, 4, 3, 2, 1] + counts, ranks = zip(*groups) + ranks = list(ranks) + if ranks == [14, 5, 4, 3, 2]: + ranks = [5, 4, 3, 2, 1]This keeps the existing logic but fixes the comparison so the wheel straight is correctly recognized.
🧹 Nitpick comments (3)
puzzles/poker/__init__.py (1)
27-61: Nested conditional preserves semantics but harms readabilityThe ranking precedence looks unchanged and correct, but the deeply nested ternary is hard to read and maintain. Consider flattening this into a straightforward
if/elifladder before returning(rank, ranks).- return ( - ( - 9 - if counts == (5,) - else ( - 8 - if straight and flush - else ( - 7 - if counts == (4, 1) - else ( - 6 - if counts == (3, 2) - else ( - 5 - if flush - else ( - 4 - if straight - else ( - 3 - if counts == (3, 1, 1) - else ( - 2 - if counts == (2, 2, 1) - else 1 - if counts == (2, 1, 1, 1) - else 0 - ) - ) - ) - ) - ) - ) - ) - ), - ranks, - ) + if counts == (5,): + score = 9 + elif straight and flush: + score = 8 + elif counts == (4, 1): + score = 7 + elif counts == (3, 2): + score = 6 + elif flush: + score = 5 + elif straight: + score = 4 + elif counts == (3, 1, 1): + score = 3 + elif counts == (2, 2, 1): + score = 2 + elif counts == (2, 1, 1, 1): + score = 1 + else: + score = 0 + + return score, ranksdatastructures/trees/binary/test_utils.py (2)
9-88: Core LCA scenarios are well covered; consider clearer test names and one extra edge caseThe four tests exercise key shapes (same subtree, cross‑subtree with root LCA, ancestor/descendant, opposite subtrees) and look consistent with the documented behavior of
lowest_common_ancestor.To improve maintainability, consider:
- Renaming
test_1–test_4to descriptive names (e.g.test_same_subtree,test_root_is_lca,test_ancestor_is_lca,test_opposite_subtrees).- Optionally adding a small test where both inputs are the same node and where the tree has a single node; this guards against regressions in trivial cases.
90-169: Good mirrored coverage for pointer variant; duplication could be reducedThese tests nicely mirror the non‑pointer suite for
lowest_common_ancestor_ptr, which is great for validating that both implementations agree.There is a lot of duplicated tree‑construction code between the two classes; you could factor out small helpers or a shared mixin (e.g.
make_tree_case_1(), etc.) to avoid copy‑pasting structures and keep future changes in sync more easily.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
datascience/numeric_python/statistics/TestChecker.py(1 hunks)datastructures/trees/binary/test_utils.py(1 hunks)puzzles/graphs/evaluate_division/test_evaluate_division.py(2 hunks)puzzles/poker/__init__.py(1 hunks)pyproject.toml(2 hunks)pystrings/hexadecimal/__init__.py(1 hunks)utils/basic/list1.py(1 hunks)utils/basic/list2.py(1 hunks)utils/basic/solution/list1.py(1 hunks)utils/basic/solution/list2.py(1 hunks)utils/basic/solution/string2.py(1 hunks)utils/basic/string2.py(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- utils/basic/list1.py
- utils/basic/solution/string2.py
- utils/basic/list2.py
- utils/basic/solution/list2.py
- datascience/numeric_python/statistics/TestChecker.py
- puzzles/graphs/evaluate_division/test_evaluate_division.py
🧰 Additional context used
🧬 Code graph analysis (1)
datastructures/trees/binary/test_utils.py (1)
datastructures/trees/binary/utils.py (2)
lowest_common_ancestor(4-42)lowest_common_ancestor_ptr(45-69)
🔇 Additional comments (6)
utils/basic/string2.py (1)
11-11: Formatting-only blank line; no behavior impactThe added blank line just separates the “Additional basic string exercises” header from the following code. No functional or API impact; looks fine.
utils/basic/solution/list1.py (1)
18-18: Whitespace tweak is harmless and acceptableThe extra blank line after the header comments is purely cosmetic and does not alter control flow or behavior. Change is fine to keep.
pyproject.toml (2)
4-4: Improve project metadata with meaningful description.Updating the project description from an empty string to a clear, concise summary is a good practice that improves discoverability and clarity for users and contributors.
15-15: Black formatter as dev dependency is appropriate.Adding Black as a dev dependency aligns with the PR's formatting improvements and is a standard practice for Python projects. The dev-only placement and semantic versioning constraint are both correct.
pystrings/hexadecimal/__init__.py (1)
31-35: LGTM! Clean formatting improvement.The added parentheses around the ternary expression improve readability by visually grouping the conditional logic. This is a reasonable cosmetic change that aligns with the PR's goal of applying formatting to the codebase.
datastructures/trees/binary/test_utils.py (1)
171-172:__main__guard is fine and convenientHaving
unittest.main()under a__main__guard is a handy way to run these tests directly without impacting the test runner.
Describe your change:
Adds algorithms to check for the lowest common ancestor of binary tree nodes. This also adds formatting to the overall codebase
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.