Add `SiblingSubgraph` view #336

lmondada · 2023-08-02T17:43:45Z

No description provided.

lmondada · 2023-08-03T07:08:26Z

I've been trying to implement the HugrView and HugrInternals traits but it's a nightmare in two ways:

getting the iterator types right,
exposing a PortView that represents the underlying portgraph -- probably need to create yet another type in portgraph that combines Regions and Subgraphs.

For now, I've added a todo flag and will leave this for later.

src/hugr/rewrite/simple_replace.rs

acl-cqc · 2023-08-03T11:16:21Z

Hmmm. Region is of course a bit of a misnomer - really, FlatRegion should be something like SiblingGraph and Region something like Descendants. Then you could call this region, or similar....

But if we want a general term, how about Area?

acl-cqc · 2023-08-03T11:17:15Z

really, FlatRegion should be something like SiblingGraph

Or maybe FlatRegionView should be ChildrenView? (Or FamilyView :-) :-) )

lmondada · 2023-08-03T11:33:01Z

I like your ideas @acl-cqc , will try to come up with a sensible naming scheme.

doug-q · 2023-08-07T08:04:16Z

I've implemented HugrView here: #355

lmondada · 2023-08-07T14:16:15Z

Hi @acl-cqc,

I've made a few changes that build on top of #347, #323 and @doug-q 's #355, I think the PR is now in much better shape:

SugHugr was renamed to SiblingSubgraph and its semantics were changed to only include the subgraph within the sibling graph and not its further descendants. This is all that I need and will make the HugrView impl much simpler.
HugrView and the hierarchy-related views were moved into a common hugr/views folder, along with the new sibling subgraph code.

Regarding change 2: I find the file organisation much cleaner this way, apologies to @ss2165 for changing this twice in a row... I hope you will agree with me, but if you dislike the additional nesting or anything else, happy to revert the change of course!

lmondada · 2023-08-07T14:23:03Z

NB (important!): do not merge this until CQCL/portgraph#100 is merged, a new portgraph is released and Cargo.toml changed to pick up the new release!

EDIT: done.

src/hugr/views/hugr.rs

src/hugr/views/hierarchy.rs

src/hugr/views/sibling.rs

acl-cqc

Ok, thanks Luca, there is some fairly serious stuff here, although the really hard work of convexity checking is in Portgraph so I'm not looking at that :-). I've quite a few comments on restructuring though and TBH I'm not certain I've grokked all of what's here (hopefully I will after I've clarified the bits I'm commenting on). So, sorry for the long review, but I reckon we can make this significantly easier to understand and reason about, here goes....

acl-cqc · 2023-08-09T13:44:55Z

src/hugr/views/sibling.rs

+//!
+//! Views into subgraphs of HUGRs within a single level of the
+//! hierarchy, i.e. within a sibling graph. Such a subgraph is
+//! given by a root node, of which all nodes in the sibling subgraph are


nit: given -> represented?

acl-cqc · 2023-08-09T13:45:32Z

src/hugr/views/sibling.rs

+//! Views into subgraphs of HUGRs within a single level of the
+//! hierarchy, i.e. within a sibling graph. Such a subgraph is
+//! given by a root node, of which all nodes in the sibling subgraph are
+//! children, as well as a subgraph boundary, defining the separation between


maybe "a set of edges forming the subgraph boundary" ?

acl-cqc · 2023-08-09T13:46:47Z

src/hugr/views/sibling.rs

+/// of the target of the edge. Source edges are not unique as they can involve
+/// copies.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Into)]
+struct BoundaryEdge(Node, Port);


I wonder if it might be better to call this by what it is (maybe "TargetPort" or "InPort") rather than by what it's used for later - hopefully the uses will make that clear...

Yes, I agree with you. I've opted instead to implement an actual boundary edge: now this struct also includes whether it's an incoming or outgoing boundary edge, and it panics if the edge does not exist.

acl-cqc · 2023-08-09T13:47:03Z

src/hugr/views/sibling.rs

+struct BoundaryEdge(Node, Port);
+
+impl BoundaryEdge {
+    fn target_port_index<G: PortView>(&self, g: &G) -> PortIndex {


maybe call this portgraph_index or pg_index

I've renamed to portgraph_target, along with a bunch of other renames.

acl-cqc · 2023-08-09T13:50:49Z

src/hugr/views/sibling.rs

+    fn target_port_index<G: PortView>(&self, g: &G) -> PortIndex {
+        let Node { index: node } = self.0;
+        let Port { offset: port } = self.1;
+        g.port_index(node, port).unwrap()


Suggested change

g.port_index(node, port).unwrap()

g.port_index(self.0.index, self.1.offset).unwrap()

Took your suggestion + some rewriting.

acl-cqc · 2023-08-09T14:45:57Z

src/hugr/views/sibling.rs

+///  - a HugrView to the underlying HUGR,
+///  - a root node, of which all nodes in the [`SiblingSubgraph`] must be
+///    children,
+///  - incoming/outgoing boundary edges, pointing into/out of the subgraph.


nit: "incoming and outgoing" - making clear there are two sets

Rewrote this.

acl-cqc · 2023-08-09T14:46:30Z

src/hugr/views/sibling.rs

+/// The incoming/outgoing edges must be contained within the sibling graph of the
+/// root node. Their ordering matters when using the [`SiblingSubgraph`] for
+/// replacements, as it will match the ordering of the ports in the replacement.
+/// The list of incoming and/or outgoing ports may be empty.


...", in which case the SiblingSubgraph contains all children of the parent."

Rewrote this.

src/hugr/views/sibling.rs

acl-cqc · 2023-08-09T15:02:40Z

src/hugr/views/sibling.rs

+    /// `replacement` must be a hugr with DFG root and its signature must
+    /// match the signature of the subgraph.
+    ///
+    /// Panics if


Note that specification/hugr.md defines SimpleReplace as requiring the replaced nodes to be convex. That check isn't implemented there, but you could check that here. (That might reduce the utility of allowing non-convex SiblingSubgraphs, too...)

I've changed create_simple_replacement to return a Result and to check that the subgraph is convex.

acl-cqc · 2023-08-21T15:56:37Z

src/hugr/views/hierarchy.rs

@@ -48,7 +48,7 @@ where
    graph: FlatRegionGraph<'g, Base>,

    /// The rest of the HUGR.
-    hugr: &'g Base,
+    pub(super) hugr: &'g Base,


You can revert this now (yay!)

acl-cqc · 2023-08-21T15:59:46Z

src/hugr/views/sibling.rs

+        });
+        let to_pg = |(n, p): (Node, Port)| pg.port_index(n.index, p.offset).expect("invalid port");
+        let subpg = Subgraph::new_subgraph(pg, incoming.chain(outgoing).map(to_pg));
+        if !subpg.is_convex_with_checker(checker) {


presumably it's more efficient to do the convexity check this way, rather than by checker.is_node_convex on nodes below ? If the latter is ok, you could replace all these last checks by returning try_new_with_checker

It's not more efficient, however doing the convexity on nodes only will miss some edge cases. Calling try_new_with_checker after the convexity check with the nodes would do the convexity check twice, so I think this is the only way.

acl-cqc

Thanks @lmondada for running the marathon :) and some of my misreadings. (Maybe this wasn't a good first-task-after-a-holiday "gearchange"!)

I'm pretty happy with this now except that I think there is an issue with state edges and I'm not sure what we should do about that here.

acl-cqc · 2023-08-21T16:12:19Z

@acl-cqc Do you think we are ready to merge? As far as I can see there is only this one question that is still open-ish

#336 (comment)

Happy to drop that one, sorry for barking up the wrong tree. I've posted 4 more comments above, 3 of which I think are pretty small/straightforward. That leaves the state edges - it's possible that the best thing to do there is to leave that as a TODO, I'm not sure.

acl-cqc · 2023-08-21T16:28:40Z

(i.e. relating to #432)

Co-authored-by: Alan Lawrence <[email protected]>

lmondada · 2023-08-21T17:11:34Z

Not at all! All your remarks and questions were very pertinent @acl-cqc. Thank you so much! This review process was very necessary and more than worth it! If anything I'm sorry it took so much of your time!

For the time being, I suggest leaving the order edges question open and merging this in. I've added a TODO for this.

We will come back once we have a clear answer, or @aborgna-q and me figure out that we need order edges after all 😛

acl-cqc · 2023-08-21T21:00:06Z

For the time being, I suggest leaving the order edges question open and merging this in. I've added a TODO for this.

What I'd suggest for the time being is to panic if we find any order edges. I think there are two cases:

Edges in/out of the subgraph. I think these would have to be part of the boundary (if you created a SiblingSubgraph from just the value edges in/out, you'd end up getting more of the graph than you intended, and probably fail the convexity/well-formedness check, right). But I think at the moment if such edges were part of the boundary, you'd filter them out, or else look for corresponding edges in the replacement?? So maybe panic rather than filter out.
Edges in/out of the replacement (i.e. from Input node to the rest, or to the Output node). You filter these out, but again, perhaps best to panic.

Other than that, I think all good, I'll have a final check through in the morning!

acl-cqc

Looks great, thanks Luca!

acl-cqc · 2023-08-22T08:18:50Z

src/hugr/views/sibling.rs

+/// The incoming boundary (resp. outgoing boundary) is given by the input (resp.
+/// output) ports of the subgraph that are linked to nodes outside of the subgraph.
+/// The signature of the subgraph is then given by the types of the incoming
+/// and outgoing boundary edges. Given a replacement with the same signature,


nit: edges -> ports? If I understand correctly and you include a node with a port that has many edges to outside the subgraph, the port gets included once, not once per edge?

acl-cqc · 2023-08-22T08:22:02Z

src/hugr/views/sibling.rs

+    ///
+    /// More formally, the sibling subgraph defined by $B_I$ and $B_O$ is the
+    /// graph given by the connected components of the graph with edges
+    /// $E\B_I\B_O$ which contain at least one node that is either


nit: note we haven't defined $E$ here! I quite liked the "connected components of the graph G'=(V, E \ B_I \ B_O)` too as it's then clearer that "which contain at least one node..." refers to the connected components rather than the edges. (But it's not really ambiguous as edges don't contain nodes so ok.)

src/hugr/views/sibling.rs

acl-cqc · 2023-08-22T08:24:13Z

src/hugr/views/sibling.rs

+            .outgoing_ports()
+            .filter_map(|(n, p)| self.base.get_optype(n).signature().get(p).cloned())
+            .collect_vec();
+        FunctionType::new(input, output)


Oh yes, you can use FunctionType - nice :)

acl-cqc · 2023-08-22T08:25:12Z

src/hugr/views/sibling.rs

+        }
+        let rep_inputs = replacement
+            .node_outputs(rep_input)
+            // filter out any non-dataflow ports


I'd probably panic here rather than filter out (then we'll know when the TODO comes into play) but ok

acl-cqc · 2023-08-22T08:26:24Z

src/hugr/views/sibling.rs

+    EmptySubgraph,
+}
+
+// /// A common trait for views of a HUGR sibling subgraph.


Probably just remove this? Unless you want to impl it for SiblingGraph and SiblingSubgraph as a no-methods marker trait

acl-cqc · 2023-08-22T08:26:58Z

src/hugr/views/sibling.rs

+        ///
+        /// This will return an [`InvalidSubgraph::EmptySubgraph`] error if the
+        /// subgraph is empty.
+        pub fn from_sibling_graph(sibling_graph: &'g Base) -> Result<Self, InvalidSubgraph>


nit: no need/point in making this pub, as tests is hidden. (For now!)

acl-cqc · 2023-08-22T08:27:56Z

src/hugr/views/sibling.rs

+
+        hugr.apply_rewrite(rep).unwrap();
+
+        assert_eq!(hugr.node_count(), 4); // Module + Def + In + Out


Would be good to have a contrasting assert before you apply the rewrite

acl-cqc · 2023-08-22T08:29:42Z

src/hugr/views/sibling.rs

+            Direction::Incoming => base.linked_ports(n, p).collect(),
+            Direction::Outgoing => vec![(n, p)],
+        });
+        let to_pg = |(n, p): (Node, Port)| pg.port_index(n.index, p.offset).expect("invalid port");


this might be a place to look for state-order edges and panic(?) if you find them. Else you might comment here that we probably need to deal with such here too (?)

Co-authored-by: Alan Lawrence <[email protected]>

Preventively fixes CI errors before rust 1.72 is released, and rustfmt starts having opinions on let-else formatting. https://blog.rust-lang.org/2023/07/01/rustfmt-supports-let-else-statements.html

aborgna-q

LGTM :)

ss2165 and others added 6 commits August 2, 2023 19:43

remove unused sealed traits (#333)

f3387d8

Add IOBlock OpTag

e2c38e0

Add SubHugr

ee4be6e

REVERTME use portgraph HEAD

c552388

Use AbstractSignature

12c9201

Add/fix tests

14ef467

acl-cqc reviewed Aug 3, 2023

View reviewed changes

src/hugr/rewrite/simple_replace.rs Outdated Show resolved Hide resolved

lmondada added 2 commits August 3, 2023 13:26

Merge branch 'main' into feature/convex

ce83f7b

Merge branch 'main' into feature/convex

3b4a3cc

lmondada force-pushed the feature/convex branch from 206577a to 3b4a3cc Compare August 3, 2023 11:30

lmondada mentioned this pull request Aug 3, 2023

Rename Region views #347

Merged

lmondada mentioned this pull request Aug 7, 2023

Add impl HugrView for SubHugr #355

Closed

lmondada added 2 commits August 7, 2023 15:04

Merge branch 'main' into feature/convex

825108d

Only consider flat subgraphs -> SiblingSubgraph

1dd3261

Fix funky rustdoc err + pub mod

d30ed89

lmondada marked this pull request as ready for review August 7, 2023 14:20

lmondada requested a review from acl-cqc August 7, 2023 14:20

lmondada changed the title ~~Add SubHugr view~~ Add SiblingSubgraph view Aug 7, 2023

acl-cqc reviewed Aug 9, 2023

View reviewed changes

src/hugr/views/hugr.rs Outdated Show resolved Hide resolved

acl-cqc reviewed Aug 9, 2023

View reviewed changes

src/hugr/views/hierarchy.rs Outdated Show resolved Hide resolved

acl-cqc reviewed Aug 9, 2023

View reviewed changes

src/hugr/views/sibling.rs Show resolved Hide resolved

lmondada mentioned this pull request Aug 9, 2023

Reorganise HUGR views #386

Merged

acl-cqc reviewed Aug 9, 2023

View reviewed changes

acl-cqc reviewed Aug 21, 2023

View reviewed changes

lmondada and others added 3 commits August 21, 2023 18:56

Update src/hugr/views/sibling.rs

8d5b751

Co-authored-by: Alan Lawrence <[email protected]>

Address more comments

456057a

Add TODO for order edges

0a31835

Merge branch 'main' into feature/convex

0314058

lmondada requested a review from acl-cqc August 21, 2023 17:12

lmondada added 2 commits August 21, 2023 19:25

Merge changes

e493888

Doc fix!

da7d3ad

acl-cqc approved these changes Aug 22, 2023

View reviewed changes

lmondada disabled auto-merge August 22, 2023 08:47

lmondada and others added 5 commits August 22, 2023 14:04

Update src/hugr/views/sibling.rs

69c9747

Co-authored-by: Alan Lawrence <[email protected]>

chore: Format let-else with nightly rustfmt (#434)

402911f

Preventively fixes CI errors before rust 1.72 is released, and rustfmt starts having opinions on let-else formatting. https://blog.rust-lang.org/2023/07/01/rustfmt-supports-let-else-statements.html

new fmt

edca9c8

Final minor adjustments

4d9d276

Merge branch 'main' into feature/convex

4ddcf3f

lmondada enabled auto-merge August 22, 2023 12:13

Filter out state order edges

539d56e

lmondada force-pushed the feature/convex branch from e583069 to 539d56e Compare August 22, 2023 12:23

Add is_order_edge

41c2bc7

aborgna-q approved these changes Aug 22, 2023

View reviewed changes

lmondada added this pull request to the merge queue Aug 22, 2023

Merged via the queue into main with commit caacecf Aug 22, 2023

lmondada deleted the feature/convex branch August 22, 2023 12:45

	g.port_index(node, port).unwrap()
	g.port_index(self.0.index, self.1.offset).unwrap()


		hugr.apply_rewrite(rep).unwrap();

		assert_eq!(hugr.node_count(), 4); // Module + Def + In + Out

Add SiblingSubgraph view #336

Add SiblingSubgraph view #336

Conversation

lmondada commented Aug 2, 2023 • edited Loading

lmondada commented Aug 3, 2023

acl-cqc commented Aug 3, 2023

acl-cqc commented Aug 3, 2023

lmondada commented Aug 3, 2023

doug-q commented Aug 7, 2023

lmondada commented Aug 7, 2023

lmondada commented Aug 7, 2023 • edited Loading

acl-cqc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc left a comment

Choose a reason for hiding this comment

acl-cqc commented Aug 21, 2023

acl-cqc commented Aug 21, 2023

lmondada commented Aug 21, 2023

acl-cqc commented Aug 21, 2023

acl-cqc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aborgna-q left a comment

Choose a reason for hiding this comment

Add `SiblingSubgraph` view #336

Add `SiblingSubgraph` view #336

lmondada commented Aug 2, 2023 •

edited

Loading

lmondada commented Aug 7, 2023 •

edited

Loading

acl-cqc Aug 21, 2023 •

edited

Loading