Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Efficient LCA algorithm for the hierarchy #138

Merged
merged 5 commits into from
Jul 2, 2024
Merged

feat: Efficient LCA algorithm for the hierarchy #138

merged 5 commits into from
Jul 2, 2024

Conversation

aborgna-q
Copy link
Collaborator

@aborgna-q aborgna-q commented Jul 1, 2024

Implement the lowest-common-ancestor algorithm on hierarchies, using the "binary lifting" algorithm.
This is needed for the mermaid rendering, but it's also a useful algorithm to have around. (There's already been talk of using it in hugr).

Precomputation time: O(n log d)
lca(a,b) query time: O(log d)
is_ancestor(a,b) query time: O(1)
Memory: O(n log d)

where n=#nodes and d=max depth.

There's also a constant time algorithm based on RMQ, but it doesn't tend to be much faster than this one. This impl is much simpler.

Copy link

codecov bot commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 94.84536% with 5 lines in your changes missing coverage. Please review.

Project coverage is 80.17%. Comparing base (b3c8142) to head (c63e8b1).
Report is 1 commits behind head on main.

Files Patch % Lines
src/algorithms/lca.rs 94.84% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
+ Coverage   79.77%   80.17%   +0.40%     
==========================================
  Files          21       22       +1     
  Lines        5043     5140      +97     
  Branches     5043     5140      +97     
==========================================
+ Hits         4023     4121      +98     
+ Misses        949      946       -3     
- Partials       71       73       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@lmondada lmondada left a comment

Choose a reason for hiding this comment

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

Very neat!

Comment on lines 64 to 75
if let Some(parent) = parent {
let mut climb = vec![parent];
let mut prev = parent;
for i in 1.. {
let Some(&u) = lca.climb_nodes[prev].get(i - 1) else {
break;
};
climb.push(u);
prev = u;
}
lca.climb_nodes[node] = climb;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a good use case for .scan, although I'm still on the fence whether .scan can ever be easy to read... Up to you!

Suggested change
if let Some(parent) = parent {
let mut climb = vec![parent];
let mut prev = parent;
for i in 1.. {
let Some(&u) = lca.climb_nodes[prev].get(i - 1) else {
break;
};
climb.push(u);
prev = u;
}
lca.climb_nodes[node] = climb;
}
let climb: Vec<_> = (1..)
.scan(parent, |state, i| {
let prev = state?;
*state = lca.climb_nodes[prev].get(i - 1);
Some(prev)
})
.collect();
if !climb.is_empty() {
lca.climb_nodes[node] = climb;
}

src/algorithms/lca.rs Show resolved Hide resolved
Comment on lines 140 to 150
loop {
let Some(&last_climb) = self.climb_nodes[u].last() else {
// We reached a root, and it is not an ancestor of `b`.
return None;
};
if self.is_ancestor(last_climb, b) {
// We found a `u` where the last ancestor is an ancestor of `b`.
break;
}
u = last_climb;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be taking functional style too far today, but consider (you'll need to import std::iter):

Suggested change
loop {
let Some(&last_climb) = self.climb_nodes[u].last() else {
// We reached a root, and it is not an ancestor of `b`.
return None;
};
if self.is_ancestor(last_climb, b) {
// We found a `u` where the last ancestor is an ancestor of `b`.
break;
}
u = last_climb;
}
// Find a `u` where the last ancestor is an ancestor of `b`.
let mut u = iter::from_fn(|| {
u = self.climb_nodes[u].last();
u
}).find(|last_climb| self.is_ancestor(last_climb, b))?;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to modify it a bit, but I like the functional style :)

Comment on lines 161 to 165
// The 2^i ancestor is not an ancestor of `b`.
// Lets update `u`.
u = v;
// Decrease `i`, and ensure it is within bounds.
i = i.max(self.climb_nodes[u].len() - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I've added some comments that made it clearer for me. You could add another comment that this is binary search and that 2^i is the size of the interval the LCA must be in.

Suggested change
// The 2^i ancestor is not an ancestor of `b`.
// Lets update `u`.
u = v;
// Decrease `i`, and ensure it is within bounds.
i = i.max(self.climb_nodes[u].len() - 1);
// The 2^i ancestor of `u` is not an ancestor of `b` so the LCA must be between
// the 2^i and 2^{i+1} ancestor of `u`. Hence update `u` to the 2^i ancestor.
u = v;
// Ensure `i` is within bounds.
i = i.max(self.climb_nodes[u].len() - 1);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍
I added more comments.

@aborgna-q aborgna-q added this pull request to the merge queue Jul 2, 2024
Merged via the queue into main with commit a5be34e Jul 2, 2024
12 checks passed
@aborgna-q aborgna-q deleted the ab/lca branch July 2, 2024 09:21
@hugrbot hugrbot mentioned this pull request Jul 2, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 5, 2024
Defines intergraph edges on the parent region, so mermaid renders them
correctly.

This required a lowest-common-ancestor implementation for the hierarchy.
See #138.
I replaced the recursive DFS with a stack-based one that reuses the
structures when exploring multiple trees.

I also added some benchmarks for both rendering algorithms.

This closes CQCL/hugr#1197
github-merge-queue bot pushed a commit that referenced this pull request Jul 5, 2024
## 🤖 New release
* `portgraph`: 0.12.1 -> 0.12.2

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [0.12.2](v0.12.1...v0.12.2)
- 2024-07-05

### Bug Fixes
- Intergraph edges in mermaid rendering
([#139](#139))

### New Features
- Add PortOffset::opposite
([#136](#136))
- Efficient LCA algorithm for the hierarchy
([#138](#138))

### Testing
- Use `insta` for mermaid/dot render tests
([#137](#137))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).
@hugrbot hugrbot mentioned this pull request Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants