feat: PersistentHugr Walker API #2168

lmondada · 2025-05-07T18:46:11Z

Closes #2074
Closes #2190

codecov · 2025-05-07T18:49:22Z

Codecov Report

Attention: Patch coverage is 96.19048% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.15%. Comparing base (70bec95) to head (cd0a1eb).

Files with missing lines	Patch %	Lines
hugr-core/src/hugr/persistent/walker/pinned.rs	93.42%	4 Missing and 1 partial ⚠️
hugr-core/src/hugr/persistent/walker.rs	98.41%	2 Missing ⚠️
hugr-core/src/hugr/persistent/state_space.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2168      +/-   ##
==========================================
+ Coverage   81.99%   82.15%   +0.15%     
==========================================
  Files         237      239       +2     
  Lines       42767    42977     +210     
  Branches    38679    38889     +210     
==========================================
+ Hits        35068    35307     +239     
+ Misses       5706     5679      -27     
+ Partials     1993     1991       -2

Flag	Coverage Δ
python	`85.34% <ø> (ø)`
rust	`81.81% <96.19%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lmondada · 2025-05-10T16:09:04Z

EDIT: resolved!

~~I'm currently unhappy with the state of the end_to_end test within the walker folder, but this can't be simplified and moved out of the repo into tests until further enhancements to the API.~~

~~It is nonetheless a very useful test to have, even in its current form.~~

~~See #2190~~

acl-cqc

Thanks @lmondada, this is great! Not what you'd call easy stuff, but great job in making it comprehensible :-). I have about 60 small suggestions, there are only two big enough to justify not immediately approving:

Have I understand PinnedWire always has at least one endpoint pinned? I wasn't clear from the doc so I had to guess a bit, if that's not right then this may need some more thought
That equivalent_descendant_ports method....what is all the direction-changing doing...

acl-cqc · 2025-05-13T10:30:24Z

hugr-core/src/hugr/persistent/walker.rs

@@ -0,0 +1,624 @@
+//! Exploration of a [`PersistentHugr`] through incremental traversal of


I agree with "exploration of a [CommitStateSpace]" just below, but a PersistentHugr is a non-conflicting subgraph of a CommitStateSpace, right?

Maybe "Explores a CommitStateSpace (and allows) to establish/identify/build a PersistentHugr by incrementally" and then either "traversing connected subgraphs" as you have here (not sure if I've quite followed that yet though) or "pinning nodes resulting from particular rewrites" ?

You're right! How do you find the following

Incremental traversal and construction of [PersistentHugr]s from [CommitStateSpace]s

?

It's hard to keep it to a high-level one-line summary

acl-cqc · 2025-05-13T10:31:47Z

hugr-core/src/hugr/persistent/walker.rs

+//! This module provides the [`Walker`] type, which enables intuitive
+//! exploration of a [`CommitStateSpace`] by traversing wires and gradually
+//! pinning (selecting) nodes that match a desired pattern. Unlike direct
+//! manipulation of a [`PersistentHugr`], the Walker presents a familiar


Not sure what this "direct manipulation of a PersistentHugr" is that is so different from the "familiar nodes-and-wires interface". PersistentHugr implements HugrView, which is exactly that familiar nodes-and-wires interface....?

Sorry, PersistentHugr -> CommitStateSpace. I've added a bit more context to this sentence, does it make sense in its new form?

hugr-core/src/hugr/persistent/walker.rs

acl-cqc · 2025-05-13T10:34:18Z

hugr-core/src/hugr/persistent/walker.rs

+//!    exploration typically branches out into several parallel walkers.
+//! 4. As walkers are expanded, more nodes get pinned, narrowing down the space
+//!    of possible HUGRs.
+//! 5. Once exploration is complete (e.g. a pattern was fully matched), the


Yes! Nice :)

acl-cqc · 2025-05-13T10:34:45Z

hugr-core/src/hugr/persistent/walker.rs

+//! 5. Once exploration is complete (e.g. a pattern was fully matched), the
+//!    walker can be converted into a [`PersistentHugr`] instance using
+//!    [`Walker::into_hugr`]. The matched nodes and ports can then be used to
+//!    create a [`SimpleReplacement`](crate::SimpleReplacement) object, which


I'll suspend disbelief here - the SimpleReplacement is the combination of all of the selected patches?

No, you've done well to highlight this point. What I intended to say is that the pinned nodes can be used to specify a subgraph of the current PersistentHugr, which in turn can be used to define a new SimpleReplacement.

acl-cqc · 2025-05-13T14:25:59Z

hugr-core/tests/walker.rs

+                    .contains(&pinned_node.1),
+                "pinned node is deleted"
+            );
+            for walker in walker.expand(&wire, None) {


Suggested change

for walker in walker.expand(&wire, None) {

for subwalker in walker.expand(&wire, None) {

acl-cqc · 2025-05-13T14:30:58Z

hugr-core/tests/walker.rs

+
+            // enqueue new wires added by the replacement
+            // (this will also add a lot of already visited wires, but they will
+            // be deduplicated)


IIUC, if you moved the outermost loop in enqueue_all to the first callsite, then you could skip that loop here because you are adding only a single replacement? Would that address this comment?

no -- you'd need to consider all commits that could be neighbours (but disjoint) with the newly added commit. It's a bit iffy, another area where the API will probably have to improve in the future.

hugr-core/tests/walker.rs

acl-cqc · 2025-05-13T14:32:59Z

hugr-core/src/hugr/persistent/walker.rs

+    }
+
+    /// Get all pinned ports of the wire.
+    pub fn all_ports(&self) -> impl Iterator<Item = (PatchNode, Port)> + '_ {


Consider calling these pinned_outport, pinned_inports and all_pinned_ports...reading the client code it's easy to miss that you have a PinnedWire not a Wire

Good names!

hugr-core/tests/walker.rs

…-impl-hugrview

acl-cqc

Great stuff Luca, looks good to me, thanks! :). I think you can take it from here, indeed I think some of my comments actually pertain to code from other approved/merged PRs rather than this one...(but the opportunity to remove IteratorNonEmpty might be best taken before we forget 😉)

acl-cqc · 2025-05-23T12:00:57Z

hugr-core/src/hugr/persistent/tests.rs

@@ -144,6 +144,65 @@ fn create_not_and_to_xor_replacement(hugr: &Hugr) -> SimpleReplacement {
    SimpleReplacement::try_new(subgraph, hugr, replacement_hugr).unwrap()
 }

+/// Creates a state space with 4 commits on top of the base hugr `simple_hugr`:


This is great, but let's just note that these are not commits are not intended to be functionally equivalent (we do not mean that a and (not b) is equivalent to a xor b)

hugr-core/src/hugr/persistent.rs

acl-cqc · 2025-05-23T12:15:12Z

hugr-core/src/hugr/persistent.rs

+            // incoming ports are of interest to us if
+            //  (i) they are connected to the output of a replacement (then there will be a
+            //      linked port in a parent commit), or
+            //  (ii) they are deleted by a child commit and is not the same as the out_node


Suggested change

// (ii) they are deleted by a child commit and is not the same as the out_node

// (ii) they are deleted by a child commit and are not (equal to) the out_node

is -> are as we are talking "they" ;). TBH I think the "equal to" is unnecessary.

This is a really great comment, though, thank you :)

hugr-core/src/hugr/persistent.rs

acl-cqc · 2025-05-23T15:18:36Z

hugr-core/src/hugr/persistent/walker.rs

+            {
+                match opp_port.as_directed() {
+                    Either::Left(opp_port) => {
+                        let Some((n, p)) = self


super-nit: might be better to if let rather than continue

acl-cqc · 2025-05-23T15:19:20Z

hugr-core/src/hugr/persistent/walker.rs

+}
+
+impl CommitStateSpace {
+    /// Given a node and port, return all children commit of the current `node`


Suggested change

/// Given a node and port, return all children commit of the current `node`

/// Given a node and port, return all child commits of the current `node`

acl-cqc · 2025-05-23T15:22:53Z

hugr-core/src/hugr/persistent/walker.rs

+
+impl CommitStateSpace {
+    /// Given a node and port, return all children commit of the current `node`
+    /// that delete `node` but do not delete a linked port of `(node, port)`.


What does a linked port of ... mean?

any port linked to ... ? (if they deleted anything linked, they are not returned)?

a specified port linked to .... ? (I don't think this is what you mean because I don't see anything specified)

all ports linked to .... (they can delete some ports linked to that, as long as there is at least one they don't delete)? I think that would be better expressed as that delete ``node`` but keep at least one port linked to ``(node, port)``

something else?

hugr-core/src/hugr/persistent/walker.rs

acl-cqc · 2025-05-23T15:31:20Z

hugr-core/src/hugr/persistent/walker.rs

+            }
+        } else {
+            let commit = self.state_space.get_commit(commit_id).clone();
+            // TODO: we should be able to check for an AlreadyPinned error at


Suggested change

// TODO: we should be able to check for an AlreadyPinned error at

// TODO/Optimize: we should be able to check for an AlreadyPinned error at

I mistakenly made these as comments on #2168 because GitHub's "show changes from last review" includes changes merged in from main - the code I was commenting on actually went into main as part of #2202 . So, nothing major, but separating out.

lmondada requested a review from a team as a code owner May 7, 2025 18:46

lmondada requested review from cqc-alec and removed request for a team May 7, 2025 18:46

lmondada marked this pull request as draft May 7, 2025 18:46

lmondada removed the request for review from cqc-alec May 7, 2025 18:46

Squashed commit of feat/persistenthugr changes

cb812be

lmondada force-pushed the feat/persistenthugr branch from a7729f3 to cb812be Compare May 8, 2025 07:05

Merge branch 'main' into feat/persistenthugr

1ab5ea7

lmondada force-pushed the feat/persistentwalker branch 2 times, most recently from fd0c21f to 3b27b4d Compare May 10, 2025 15:51

lmondada marked this pull request as ready for review May 10, 2025 15:52

lmondada force-pushed the feat/persistentwalker branch 2 times, most recently from 2206368 to b81ed94 Compare May 10, 2025 16:07

lmondada force-pushed the feat/persistentwalker branch 2 times, most recently from 782e1ba to ca5d841 Compare May 10, 2025 21:43

lmondada added 3 commits May 12, 2025 10:42

Merge branch 'main' into feat/persistenthugr

6723c28

Merge branch 'main' into feat/persistenthugr

1771404

feat: PersistentHugr implements HugrView

7cd4af7

lmondada force-pushed the feat/persistentwalker branch from ca5d841 to 7874237 Compare May 12, 2025 14:37

lmondada changed the base branch from feat/persistenthugr to luca/persistent-impl-hugrview May 12, 2025 14:43

lmondada force-pushed the feat/persistentwalker branch from 7874237 to b1685fd Compare May 12, 2025 14:44

lmondada requested a review from acl-cqc May 12, 2025 14:45

lmondada added 2 commits May 12, 2025 18:06

cargo fmt

d04d1f6

test coverage ↗️

08e3eb0

lmondada force-pushed the feat/persistentwalker branch from b1685fd to 654c4fc Compare May 12, 2025 16:37

feat: PersistentHugr Walker API

b4bae59

lmondada force-pushed the feat/persistentwalker branch from 654c4fc to b4bae59 Compare May 12, 2025 16:43

acl-cqc reviewed May 13, 2025

View reviewed changes

lmondada added 4 commits May 14, 2025 12:50

address comments

13caf29

feat: Add boundary edge traversal in SimpleReplacement

1a81801

remove stray import

fc61376

feat: PersistentHugr implements HugrView

7cd85d7

lmondada force-pushed the luca/persistent-impl-hugrview branch from 08e3eb0 to 7cd85d7 Compare May 19, 2025 11:14

lmondada added 6 commits May 20, 2025 10:28

make functions fallible

52e722b

Merge branch 'luca/add-bd-traversal-simple-repl' into luca/persistent…

3136e17

…-impl-hugrview

Merge branch 'main' into luca/persistent-impl-hugrview

4e55567

Merge branch 'luca/persistent-impl-hugrview' into feat/persistentwalker

728d914

simplify equivalent_descendant_ports

6c102ee

fix test and compilation error

fc0aa0c

lmondada requested a review from acl-cqc May 21, 2025 07:30

fix outdated doc hyperlinks

1e2e05e

Base automatically changed from luca/persistent-impl-hugrview to main May 22, 2025 14:34

Merge branch 'main' into feat/persistentwalker

cd0a1eb

acl-cqc approved these changes May 23, 2025

View reviewed changes

acl-cqc mentioned this pull request May 23, 2025

refactor: tidies/readability improvements to PersistentHugr #2251

Merged

		@@ -0,0 +1,624 @@
		//! Exploration of a [`PersistentHugr`] through incremental traversal of

	for walker in walker.expand(&wire, None) {
	for subwalker in walker.expand(&wire, None) {

	// (ii) they are deleted by a child commit and is not the same as the out_node
	// (ii) they are deleted by a child commit and are not (equal to) the out_node

	/// Given a node and port, return all children commit of the current `node`
	/// Given a node and port, return all child commits of the current `node`

	// TODO: we should be able to check for an AlreadyPinned error at
	// TODO/Optimize: we should be able to check for an AlreadyPinned error at

feat: PersistentHugr Walker API #2168

Are you sure you want to change the base?

feat: PersistentHugr Walker API #2168

Conversation

lmondada commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lmondada commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acl-cqc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

acl-cqc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lmondada commented May 7, 2025 •

edited

Loading

codecov bot commented May 7, 2025 •

edited

Loading

lmondada commented May 10, 2025 •

edited

Loading