Fix LCA greedy descent returning non-lowest common ancestor#8530
Closed
bysiber wants to merge 1 commit intonetworkx:mainfrom
Closed
Fix LCA greedy descent returning non-lowest common ancestor#8530bysiber wants to merge 1 commit intonetworkx:mainfrom
bysiber wants to merge 1 commit intonetworkx:mainfrom
Conversation
The greedy descent from an arbitrary common ancestor follows
successors that are also common ancestors. But when the path from
a root to the true LCA passes through nodes that are ancestors of
only one target (not common ancestors), the descent gets stuck and
returns a non-lowest ancestor.
Example: in a diamond DAG 0→1→3→5, 0→2→4→5, 5→6, 5→7, the
common ancestors of (6,7) are {0,5}. Starting at 0, neither
successor (1,2) is a common ancestor, so the descent stops at 0.
But 5 is the correct LCA.
Fix: use the subgraph of common ancestors and find a sink node
(out-degree 0), which is guaranteed to be a lowest common ancestor.
rossbar
reviewed
Feb 20, 2026
Contributor
rossbar
left a comment
There was a problem hiding this comment.
Closing per the agentic PR policy
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.
Summary
The greedy descent in
_generate_lca_from_pairscan return a non-lowest common ancestor when the path from a root ancestor to the true LCA passes through nodes that are not common ancestors of both targets.Problem
The current algorithm picks an arbitrary common ancestor and greedily descends by following successors that are also common ancestors. However, in certain DAG topologies the immediate successors of a higher common ancestor are not themselves common ancestors — they're only ancestors of one target. This causes the descent to get stuck and return a higher (non-lowest) ancestor.
Minimal example:
Node 0 is a common ancestor of 6 and 7. The greedy descent tries successors of 0 (nodes 1, 2), but neither is a common ancestor of both 6 and 7 — node 1 reaches only 6 via 3→5→6, and node 2 reaches only 7 via 4→5→7. The descent stops at 0 and incorrectly returns it as the LCA.
Fix
Instead of greedy descent, build a subgraph of all common ancestors and find a sink node (out-degree 0 within that subgraph). A sink in the common-ancestor subgraph is by definition a lowest common ancestor — it has no common-ancestor descendants, which is exactly the LCA criterion.
This approach is correct for any DAG topology since it doesn't depend on the local greedy property holding at every step.