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

Update definition of Replace (and clarify SimpleReplace) #596

Merged
merged 18 commits into from
Oct 11, 2023

Conversation

acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Oct 9, 2023

  • Clarify various characteristics of mu_in and mu_out maps in SimpleReplace
  • Remove definitions (inc duplicate) of Partial Hugr and Separated (inlining into single use)
  • Replace requires unique parent node P, removes set of children of P, adds new subtrees only as children of P (so drop T map)
  • nu_in/nu_out use $\Gamma \setminus R$ rather than $\Gamma \setminus S^*$
  • New children of $P$ added at positions vacated by removed children where possible
  • Extend/clarify definition of what can be missing in the RHS, and what edges can be in the nu_in/nu_out

@acl-cqc acl-cqc requested a review from cqc-alec October 9, 2023 15:32
Given a `Const<T>` node `c` and a DSG `P`, add `c` as a child of `P`,
inserting an Order edge from the Input under `P` to `c`.
Given a `Const<T>` node `c` and a container node `P` (either a `Module`,
a `CFG` node or a dataflow container), add `c` as a child of `P`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not as closely related, but Const nodes can go anywhere according to the Scoped Definitions section, and the lack of order edge follows #447 and #468

specification/hugr.md Outdated Show resolved Hide resolved
- a map $\nu\_\textrm{inp}: \textrm{inp}\_H(T \setminus \\{\texttt{Input}\\}) \to \textrm{inp}\_{\Gamma}(S)$;
- a map $\nu_\textrm{out}: \textrm{out}_{\Gamma}(S) \to \textrm{out}_H(T \setminus \\{\texttt{Output}\\})$.
- a map $\nu\_\textrm{inp}: \textrm{inp}\_H(T \setminus \\{\texttt{Input}\\}) \to \textrm{inp}\_{\Gamma}(S)$ (*note* in order to produce a valid Hugr, all keys must be present; and all possible values must be present exactly once unless Copyable);
- a map $\nu_\textrm{out}: \textrm{out}_{\Gamma}(S) \to \textrm{out}_H(T \setminus \\{\texttt{Output}\\})$ (*note* (similarly) in order to produce a valid hugr, all keys $\textrm{out}_{\Gamma}(S)$ must be present; and each possible value must be either Copyable and/or present exactly once. Any possible value not present could just be omitted from $H$...).
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are quite inconsistent with capitalization of hugr/Hugr/HUGR, but this can be fixed later.

@cqc-alec
Copy link
Collaborator

cqc-alec commented Oct 9, 2023

This seems like a useful set of clarifications, though more changes may be needed...

@acl-cqc acl-cqc added the spec Issues to do with the specification document(s) label Oct 10, 2023
@acl-cqc acl-cqc changed the title Spec updates - mostly re. SimpleReplace Update definition of Replace (and clarify SimpleReplace) Oct 10, 2023
@acl-cqc
Copy link
Contributor Author

acl-cqc commented Oct 10, 2023

Ok, I've had a pass at updating Replace.

  • Inlined definition of partial Hugr, and tried to make it a lot more precise
  • Also inlined definition of separated, as now we have single-parent restriction, separated is only used in one place
  • I'm not sure about treatment of Input/Output or Exit BasicBlock, also what if the parent $P$ is a Module node....next

specification/hugr.md Outdated Show resolved Hide resolved
specification/hugr.md Outdated Show resolved Hide resolved
specification/hugr.md Outdated Show resolved Hide resolved
- a list $\mu\_\textrm{out}$ of `NewEdgeSpec` which all have their `SrcNode`in
$G$ and `TgtNode` in $\Gamma \setminus S^*$ (and `TgtNode` has an existing
$G$ and `TgtNode` in $\Gamma \setminus R$ (and `TgtNode` has an existing
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to be more precise about the target port/whatever having an existing incoming edge, but that's a little awkward. I'm still wondering about a bigger restructure of edges from being two "lists", but I think the "well-formedness" requirements are so complex anyway that one might as well give up....

One thing I think one might want is to add new edges between nodes in `$\Gamma \setminus R$ i.e. where the source has been removed, the "new" source might also be in the remainder rather than new in the replacement. Forcing all such edges to be "passed through" Input -> Output doesn't work if $P$ isn't a DFG, and doesn't work well for static/order edges even for DFGs.

One possibility might be to allow mu_out to have any SrcNode, although that's tricky because until we've copied $G$ into $\Gamma$ we don't have a consistent notion of ID, hmmm.

A third set of new edges?? There may be call for being able to add arbitrary new Order edges, too....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course one can always use HugrMut::connect, I guess we can add this later if we really find a need

@acl-cqc acl-cqc requested a review from cqc-alec October 10, 2023 15:46
specification/hugr.md Outdated Show resolved Hide resolved
- a list $\mu\_\textrm{inp}$ of `NewEdgeSpec` which all have their `TgtNode`in
$G$ and `SrcNode` in $\Gamma \setminus S^*$;
$G$ and `SrcNode` in $\Gamma \setminus R$;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this change is allowing SrcNode to be in S* ∩ X*. OK I think I see why this is allowed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you move a subtree from the old Hugr to a new location....

  • There won't be any Ext edges out of it (because they'd only go to siblings, i.e. new bits added INTO the subtree you've moved, and Replace doesn't let you add any such)
  • But you might need to add Ext edges into it (if the bit you kept, had an Ext edge from a bit you removed)
  • And, you could have Dom edges out of the bit you're moving into new nodes, or Dom edges in as previous point.

Should I expand, do you think? I admit that change looks quite small but has bigger ramifications!

@@ -1253,8 +1261,10 @@ The new hugr is then derived as follows:
copy of the `SrcNode` of $\sigma\_\mathrm{out}$ according to the specification $\sigma\_\mathrm{out}$.
The target port must have an existing edge whose source is in $R$; this edge
is removed.
4. For each $(n, t = T(n))$, append the copy of $n$ to the list
of children of $t$ (adding a hierachy edge from $t$ to $n$).
4. Let $N$ be the ordered list of the copies made in $\Gamma$ of the children of the root node of $G$.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit more complex than append-to-end, but I think it may be necessary for replacing particular children of Conditionals (even without changing their number); CFG entry nodes; and Input/Output nodes. Generally you'll need to provide exactly one Input if-and-only-if you remove an Input, sort of thing, but it seems reasonable that you might???

@acl-cqc acl-cqc added this pull request to the merge queue Oct 11, 2023
Merged via the queue into main with commit 903350f Oct 11, 2023
6 checks passed
@acl-cqc acl-cqc deleted the spec/simple_replace branch October 11, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Issues to do with the specification document(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants