Define the `SimpleReplace` API. #85

cqc-alec · 2023-05-22T16:06:47Z

No description provided.

specification/hugr.md

acl-cqc · 2023-05-23T09:03:50Z

specification/hugr.md

+The method takes as input:
+
+  - the ID of a DFG node $P$ in $\Gamma$;
+  - a DFG-convex set $S$ of IDs of leaf nodes that are children of $P$;


specifically excluding Input and Output?

Yes, sorry should have added this.

acl-cqc · 2023-05-23T09:05:00Z

specification/hugr.md

+
+  - the ID of a DFG node $P$ in $\Gamma$;
+  - a DFG-convex set $S$ of IDs of leaf nodes that are children of $P$;
+  - a "modular hugr" with DFG root $R$ and only leaf nodes as children;


Definitely don't see any need for the restriction that all children should be leaf nodes; they could be containers with all their children specified too? Unless it eases implementation a lot?

Indeed, this restriction could be removed.

acl-cqc

Having a simplified version that doesn't deal (much) with hierarchy seems like a great idea :-).

I presume this is still for HUGR-with-copy-nodes? I'm a bit confused about the \nu-inp/out maps, but I think that'll change quite a bit when we have "no-copy", so maybe best to have another think then. (I'm not sure but my confusion might stem from having two definitions for $\mathrm{inp}(P)$ or $(S)$, for a parent node vs a subset of children, that look different - maybe they aren't actually different tho...

cqc-alec · 2023-05-23T09:55:14Z

I presume this is still for HUGR-with-copy-nodes?

No, this is for HUGR without copy nodes, but with potentially multiple outputs from a port. That's why the definitions of inp() and out() and the specifed maps (no longer bijective, note) look a bit different from the existing API (and another reason not to merge this into the spec yet). I had to draw some pictures to figure out what made sense here, but I hope it does.

On leaf-node restrictions: I think leaf nodes are enough for our immediate purposes, but maybe when implementing we can consider how much it complicates it to remove one or both constraints.

acl-cqc · 2023-05-23T10:05:33Z

specification/hugr.md

+  - adding all children of $R$, and all edges between them, to $\Gamma$;
+  - making $P$ the parent of all the newly added nodes except for the Input and
+    Output nodes;
+  - for each $p \in \textrm{inp}(S)$, adding an edge from the predecessor of $p$


Ah ok I'd missed the mention of "multiports". Right. So - $\nu_\textrm{inp} needs to include an entry for each linear thing in \textrm{inp}(S). Any classical inputs to \textrm{inp}(S) that are not in \nu_inp can be discarded.

A non-simple replace could allow classical values from outside the replaced graph - whether inputs to the replaced graph or not! - to be wired into (perhaps multiple) inputs of the replacement. For which one way might be to invert the map (key by inputs to R, value=what thing to wire in), although this makes it harder to check that we preserve linearity. Sorry, this thinking belongs somewhere else...

I just realized there's a mistake in the directions of the maps, will update presently.

Not to be merged since it assumes multiports, which are not yet reflected in the spec.

acl-cqc · 2023-06-05T10:13:33Z

specification/hugr.md

+  - the ID of a DFG node $P$ in $\Gamma$;
+  - a DFG-convex set $S$ of IDs of leaf nodes that are children of $P$ (not
+    including the Input and Output nodes);
+  - a hugr whose root is a DFG node $R$ with only leaf nodes as children;


just phrasing-wise, I think I'd prefer "a hugr $R$ whose root is a DFG node and which has only leaf nodes as children"

acl-cqc · 2023-06-05T10:23:48Z

specification/hugr.md

+
+Given a DFG node $P$, let:
+
+  - $\textrm{inp}(P)$ be the set of successor ports of edges from output ports


Ok, so this is actually $\textrm{inp}(S)$ (the other definition) taking S as children(P) \ {Input}, is that right? (I've been looking for a way to unify these two, or else give them different names. Maybe that's it. Ah, it doesn't account for intergraph edges - the inp(P) here would not include intergraph edges coming in, whereas inp(S) would include them.)

acl-cqc · 2023-06-05T10:25:37Z

specification/hugr.md

+
+  - $\textrm{inp}(P)$ be the set of successor ports of edges from output ports
+    of the Input node of $P$;
+  - $\textrm{out}(P)$ be the set of input ports of the Output node of $P$.


...and that's equivalent to $\textrm{out}(S)$ (the other out) taking S as the singleton set of the Output node of P

acl-cqc · 2023-06-05T10:27:33Z

specification/hugr.md

+
+The new hugr is then derived by:
+
+  - adding all children of $R$, and all edges between them, to $\Gamma$;


I think you do not want to add the new Input and Output nodes from $R$

Oh, sorry, you remove these at the end, but maybe it makes sense just not to add them

Yes, in fact in the implementation I didn't add them. I will update the spec. (Just have to be careful about distinguishing the nodes of R from the newly-added copies.)

Done in 7d1535e .

acl-cqc

Thanks Alec. The input and output maps make sense now, nice :).

I would still like to see if we can include entire subtrees rather than just leaves. If it complicates much then OK but I think that the only issue is....
Intergraph edges. I think we need to do something here, even if just to rule them out. Note that these will necessarily be edges from the outside into replaced subtrees. Since intergraph edges must be classical and can just be deleted, it feels like this shouldn't be a problem, and we might just fix this by appropriately defining the two $\mathrm{inp}$ (which as I commented might be combined and that might deal with this)
Does it make sense to restrict these to DFGs? Basically the same routine could be used for CFGs: the replacement CFG would have an entry node (just a BB) and a distinguished exit node; the in/out mapping would route control edges incoming to the replaced subgraph all to successors of the "fake" entry node, and edges to the new/fake exit node, to successors of the replaced subgraph. So there are some terminology changes but the only real thing here is that we could potentially have two sets of incoming edges (control and data), and the same for outgoing - does that mean two inp/out maps or one combined map? Or, if this is best left as DFG only, maybe we should call it dfg_replace?

acl-cqc · 2023-06-05T11:32:11Z

Thanks Alec. The input and output maps make sense now, nice :).

I would still like to see if we can include entire subtrees rather than just leaves. If it complicates much then OK but I think that the only issue is....

To summarize discussion just now:-

We believe intergraph edges "just work". The replacement will have to include corresponding endpoints, with edges to/from the Input/Output node, but these edges can then be rerouted to/from the original intergraph destinations/targets, just like other edges in/out of the replacement.
However, dealing with non-leaf nodes does complicate the implementation, so let's skip that (for now or evermore)
And, since we are dealing only with leaf nodes, CFGs are out-of-scope.

acl-cqc · 2023-06-05T11:44:13Z

Ah - intergraph edges don't "just work" - for outgoing edges we would also need to deal with the associated Order edge. I guess they would work for incoming edges (the Order edge goes to the parent of the replaced nodes, which stays in the Hugr), but uniformity may be better than expressivity here....

acl-cqc

Nice! :-)

acl-cqc · 2023-06-05T13:06:24Z

specification/hugr.md

+
+Given a set $S$ of nodes in a hugr $H$, let:
+
+  - $\textrm{inp}_H(S)$ be the set of input ports of nodes in $S$ whose source


Oh, neat - love the subscripting

acl-cqc · 2023-06-05T13:06:53Z

specification/hugr.md

+  - a hugr $H$ whose root is a DFG node $R$ with only leaf nodes as children --
+    let $T$ be the set of non-Input/Output children of $R$;
+  - a map $\nu_\textrm{inp}: \textrm{inp}_H(T) \to \textrm{inp}_{\Gamma}(S)$;
+  - a map $\nu_\textrm{out}: \textrm{out}_{\Gamma}(S) \to \textrm{out}_H(R)$.


Does the final one there need to be \textrm{out}_H(T) rather than (R) ?

or a T that includes the Input (but not the Output) - maybe we need two different T's rather than a common one

acl-cqc · 2023-06-05T13:10:36Z

specification/hugr.md

+    Ext edges;
+  - a hugr $H$ whose root is a DFG node $R$ with only leaf nodes as children --
+    let $T$ be the set of non-Input/Output children of $R$;
+  - a map $\nu_\textrm{inp}: \textrm{inp}_H(T) \to \textrm{inp}_{\Gamma}(S)$;


I wonder if we should allow the T here to include the Output node. In fact maybe we must - supposing the replacement had an Input linked directly to the Output?

I wonder if we should allow the T here to include the Output node. In fact maybe we must - supposing the replacement had an Input linked directly to the Output?

Ah, good point. An edge case I had not considered...

lol - well, me neither! But I think it's what you get if you remove a double-CX, say

I think this was why I defined the RemoveIdentity primitive. Somehow I assumed it wasn't needed in this case but I think I was wrong. Perhaps the easiest thing is to disallow this situation for for SimpleReplace, but add an implementation of RemoveIdentity. (I think if T includes the Output node it messes up everything else.)

If you keep T (=without Input or Output) in both places and add RemoveIdentity, that works. But how about

let $C_R$ be the children of $R$; - a map $\nu_\textrm{inp}: \textrm{inp}_H(C_R - \texttt{Input}) \to \textrm{inp}_{\Gamma}(S)$ - a map $\nu_\textrm{out}: \textrm{out}_{\Gamma}(S) \to \textrm{out}_H(C_R - \texttt{Output})$.

when you say it messes up everything else, what goes wrong?

That might just be a stroke of genius.

I think the trick here is working out which bits are symmetric for inputs vs outputs, and which bits don't change - like that both use "predecessors" and there is no use of "successors", which (yours) might have been the first such stroke :)

acl-cqc

Looks great :), good latex/houdini work ;)

aborgna-q marked this pull request as draft May 22, 2023 16:08

acl-cqc reviewed May 23, 2023

View reviewed changes

specification/hugr.md Show resolved Hide resolved

acl-cqc reviewed May 23, 2023

View reviewed changes

cqc-alec force-pushed the simple-replace branch from 05b5b73 to 946c45d Compare May 23, 2023 10:51

cqc-alec force-pushed the main branch from 28b8b89 to f6f87c6 Compare May 31, 2023 09:25

cqc-alec force-pushed the simple-replace branch 2 times, most recently from 9f0075b to ced917c Compare May 31, 2023 14:54

cqc-alec marked this pull request as ready for review May 31, 2023 14:55

cqc-alec requested a review from acl-cqc May 31, 2023 14:55

cqc-alec and others added 4 commits June 2, 2023 16:06

[DONOTMERGE] Define the SimpleReplace API.

7c00a7a

Not to be merged since it assumes multiports, which are not yet reflected in the spec.

Correct order of maps.

2b49af4

Exclude the input and output nodes under P.

9e97fa2

Clarify wording.

d06a623

cqc-alec mentioned this pull request Jun 5, 2023

Implement SimpleReplace #114

Merged

acl-cqc added the spec Issues to do with the specification document(s) label Jun 5, 2023

acl-cqc reviewed Jun 5, 2023

View reviewed changes

Simplify description of procedure.

7d1535e

cqc-alec force-pushed the simple-replace branch from ced917c to 7d1535e Compare June 5, 2023 11:06

Clarifications and simplifications.

4024fcf

cqc-alec requested a review from acl-cqc June 5, 2023 11:53

acl-cqc approved these changes Jun 5, 2023

View reviewed changes

cqc-alec added 3 commits June 5, 2023 14:30

Fix up latex.

560df59

Fix braino.

c9f2fab

Fix to handle empty wires.

b2dcba3

acl-cqc approved these changes Jun 5, 2023

View reviewed changes

cqc-alec merged commit ad8a00b into main Jun 5, 2023

cqc-alec deleted the simple-replace branch June 5, 2023 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define the `SimpleReplace` API. #85

Define the `SimpleReplace` API. #85

cqc-alec commented May 22, 2023 •

edited

Loading

acl-cqc May 23, 2023

cqc-alec May 23, 2023

acl-cqc May 23, 2023 •

edited

Loading

cqc-alec May 23, 2023

acl-cqc left a comment

cqc-alec commented May 23, 2023

acl-cqc May 23, 2023

cqc-alec May 23, 2023

acl-cqc Jun 5, 2023

acl-cqc Jun 5, 2023

acl-cqc Jun 5, 2023

acl-cqc Jun 5, 2023

acl-cqc Jun 5, 2023

cqc-alec Jun 5, 2023

cqc-alec Jun 5, 2023

acl-cqc left a comment

acl-cqc commented Jun 5, 2023

acl-cqc commented Jun 5, 2023

acl-cqc left a comment

acl-cqc Jun 5, 2023

acl-cqc Jun 5, 2023

acl-cqc Jun 5, 2023

acl-cqc Jun 5, 2023

cqc-alec Jun 5, 2023

acl-cqc Jun 5, 2023

cqc-alec Jun 5, 2023

acl-cqc Jun 5, 2023

cqc-alec Jun 5, 2023

acl-cqc Jun 5, 2023

acl-cqc left a comment


		Given a DFG node $P$, let:

		- $\textrm{inp}(P)$ be the set of successor ports of edges from output ports


		The new hugr is then derived by:

		- adding all children of $R$, and all edges between them, to $\Gamma$;


		Given a set $S$ of nodes in a hugr $H$, let:

		- $\textrm{inp}_H(S)$ be the set of input ports of nodes in $S$ whose source

Define the SimpleReplace API. #85

Define the SimpleReplace API. #85

Conversation

cqc-alec commented May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc May 23, 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

cqc-alec commented May 23, 2023

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 left a comment

Choose a reason for hiding this comment

acl-cqc commented Jun 5, 2023

acl-cqc commented Jun 5, 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

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

Define the `SimpleReplace` API. #85

Define the `SimpleReplace` API. #85

cqc-alec commented May 22, 2023 •

edited

Loading

acl-cqc May 23, 2023 •

edited

Loading