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: Simplify the circ hash code. Allow nodes with children. #202

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

aborgna-q
Copy link
Collaborator

Before this change, we stored the hash for each port in a map. We could then drop it when consumed if the port was linear.
This wasn't really necessary; the memory usage reduction vs just storing all the node hashes is quite small in comparison with the circuit size, and the extra logic made things slower.

This PR replaces that logic with a straight "compute each node hash and store it". This is more robust in cases that have non-local edges.

I also added a recursive call to support hashing dataflow container nodes.

- Some base types were moved from `hugr::hugr::*` to the crate's root.
@aborgna-q aborgna-q requested a review from acl-cqc October 27, 2023 12:56
Copy link
Contributor

@acl-cqc acl-cqc left a comment

Choose a reason for hiding this comment

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

Nice! :) Hashing the container is easy, and I totally agree that the linear/non-linear is not carrying its (substantial!) weight

}
#[inline]
fn set_hash(&mut self, node: Node, hash: u64) {
self.hashes.insert(node, hash);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to check the value returned from insert was None

if circ.children(node).count() > 0 {
panic!("Cannot hash container node {node:?}.");
let container: SiblingGraph =
SiblingGraph::try_new(circ, node).expect("Cannot hash container node");
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICS SiblingGraph does not insist its root node is a container; if its just a NodeHandle for a non-container, you'll get back a SiblingGraph with just one node (the root). So IIUC the only case where try_new will return a failure here is if node is bad, so you might tweak this message (or just unwrap!)

panic!("Cannot hash container node {node:?}.");
let container: SiblingGraph =
SiblingGraph::try_new(circ, node).expect("Cannot hash container node");
container.circuit_hash().hash(&mut hasher);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just in passing but I have to say - wtf, the method is u64.hash(&mut hasher) not FxHasher64.hash(u64) ?!

@@ -38,10 +37,12 @@ where
.filter(|&n| n != self.root())
{
let hash = hash_node(self, node, &mut hash_state);
hash_state.set_node(self, node, hash);
hash_state.set_hash(node, hash);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I make a suggestion here? Just rename hash_state to node_hashes. Clarify that there is no state there apart from the node hashes, which is critical in ensuring the behaviour of the hash function is not affected by e.g. the particular topsort order that we get

Base automatically changed from chore/update-deps to main October 31, 2023 12:32
@aborgna-q aborgna-q added this pull request to the merge queue Oct 31, 2023
Merged via the queue into main with commit 3007f82 Oct 31, 2023
@aborgna-q aborgna-q deleted the fix/simpler-circ-hash branch October 31, 2023 13:36
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.

None yet

2 participants