-
Notifications
You must be signed in to change notification settings - Fork 6
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
Conversation
- Some base types were moved from `hugr::hugr::*` to the crate's root.
There was a problem hiding this 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
src/circuit/hash.rs
Outdated
} | ||
#[inline] | ||
fn set_hash(&mut self, node: Node, hash: u64) { | ||
self.hashes.insert(node, hash); |
There was a problem hiding this comment.
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
src/circuit/hash.rs
Outdated
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"); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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)
?!
src/circuit/hash.rs
Outdated
@@ -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); |
There was a problem hiding this comment.
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
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.