feat!: Remove boundary map in SimpleReplacement #2208
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2208 +/- ##
==========================================
- Coverage 82.10% 82.06% -0.05%
==========================================
Files 230 230
Lines 40943 40905 -38
Branches 37044 37006 -38
==========================================
- Hits 33617 33568 -49
- Misses 5362 5369 +7
- Partials 1964 1968 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
lmondada
changed the title
lmondada
force-pushed
the
feat/simpfliy-simple-replace
branch
2 times, most recently
from
0eed63d to
8c0f118
Compare
lmondada
changed the title
lmondada
force-pushed
the
feat/simpfliy-simple-replace
branch
from
8c0f118 to
7ae8088
Compare
this would prevent say using the same replacement with different maps for alternative wirings. Perhaps we should offer a utility that can wrap a replacement with the require re-wiring/copy/discards? Urgency of this depends on how breaking this PR is downstream. |
| let subgraph_sig = subgraph.signature(host); | ||
| let repl_sig = replacement | ||
| .inner_function_type() | ||
| .expect("replacement is a DFG"); |
There was a problem hiding this comment.
this seems like something that should be reported as InvalidReplacement
| }); | ||
|
|
||
| // The incoming ports at the input boundary of `replacement` | ||
| let [repl_inp, _] = self.get_replacement_io().expect("replacement is a DFG"); |
There was a problem hiding this comment.
"replacement is a DFG" is checked at construction so get_replacement_io should be
infallible IMO (move the expect inside the call)
| .incoming_ports() | ||
| .iter() | ||
| .map(|node_ports| { | ||
| let (node, port) = *node_ports.first().expect("non-empty boundary partition"); |
There was a problem hiding this comment.
should "empty boundary partition" be prevented as an error on construction?
There was a problem hiding this comment.
Yep. This is already the case: it is prevented at construction time by SiblingSubgraph (see validate_subgraph) function in sibling_subgraph.rs.
| hugr.get_optype(node).port_kind(port).unwrap().is_value() | ||
| } |
There was a problem hiding this comment.
the unwrap is scary
| hugr.get_optype(node).port_kind(port).unwrap().is_value() | |
| } | |
| hugr.get_optype(node).port_kind(port).as_ref().is_some_and(EdgeKind::is_value) | |
| } |
| assert_eq!(h.entry_descendants().count(), 6); | ||
| } | ||
|
|
||
| #[rstest] |
There was a problem hiding this comment.
what's happened to these two tests
There was a problem hiding this comment.
They had been added in #2151 and covered the OutputBoundaryMap struct (now deleted), as well as the multiple ways in which the boundary map could be defined. I've now realised that we can get rid of all of that.
| replacement: n, | ||
| nu_inp, | ||
| nu_out: nu_out.into(), | ||
| // nu_inp, |
There was a problem hiding this comment.
done, sorry for missing these
| } | ||
| // Check that every outgoing port of a node in the subgraph whose target is not in the subgraph | ||
| // belongs to outputs. | ||
| // Check that every outgoing port of a node in the subgraph whose target is not |
There was a problem hiding this comment.
nit: lots of noise from docstring re-formatting, would have been nicer as a pre-cursor
There was a problem hiding this comment.
sorry, bad github pre-commit formatting on my side, I've undone these changes. The diff should look clean now.
I like this idea a lot. I would imagine something like: impl Hugr {
/// Change the function type of a DFG HUGR by reordering/copying/discarding
/// inputs and outputs of self.
///
/// Both `new_to_old_inputs` and `old_to_new_outputs` must be bijective when
/// restricted to the set of linear values.
fn change_function_type(
&self,
new_to_old_inputs: impl Fn(OutgoingPort) -> OutgoingPort,
old_to_new_outputs: impl Fn(IncomingPort) -> IncomingPort,
) -> Hugr {
}
}I think it makes more sense to have this in a separate PR. Currently, all instances of |
lmondada
force-pushed
the
feat/simpfliy-simple-replace
branch
from
79135c6 to
5af940a
Compare
lmondada
force-pushed
the
feat/simpfliy-simple-replace
branch
from
4b77070 to
c0d0119
Compare
The boundary map was [recently removed](#2208) in `SimpleReplacement`. Whereas previously, a boundary map could be defined to e.g. permutate the inputs and outputs between the subgraph and the provided replacement, the replacement must now match the subgraph function type as is. This PR adds a `map_function_type` method on DFG HUGRs that can be used to reorder and copy/discard inputs and outputs to the DFG pointed by the entrypoint. For the HUGR to be valid, it is also necessary to update the signatures and links in any enclosing DFGs in the ancestors of the entrypoint. Closes #2211 --------- Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
3 participants
SimpleReplacementis now so much simpler!!I've just gone ahead and removed the
nu_inpandnu_outmaps ofSimpleReplacement. Makes defining replacements so much less verbose! But you will probably (rightfully!) be sceptical that this can work.The crux is: there is already a boundary map defined in the
subgraph: SiblingSubgraph. My claim is: We can restrictnu_inpandnu_outto be bijective maps between the boundary as defined bySiblingSubgraphand the ports at the Input/Output nodes of the replacement. And once the map is bijective, we might as well restrict it to the canonical identity map, mapping the i-th port in the subgraph to the i-th port in the replacement.To convince yourself of that:
nu_inp: (Node, IncomingPort) -> (HostNode, IncomingPort)
Non-injective case: that’s using a value from the host HUGR multiple times (copying). This is covered by having a
replacementHUGR that copies a value at the input.Non-surjective case: that’s discarding a value from host. This is covered by having a
replacementthat discards a value at the input.nu_out: (HostNode, IncomingPort) -> (Node, IncomingPort)
Non-injective case: that’s using a value produced by
replacementmultiple times in the host HUGR. This is replaced by copying the value in the replacement to multiple outputsNon-surjective case: that’s discarding a value from
replacement. Replaced by not linking that value to the output node."The proof is convincing, but what about the following case?"
Consider a subgraph given by nodes
1and2and consider a wire with an outgoing port in1and two incoming ports, one in2and one in3.flowchart LR subgraph G["Subgraph"] 1 2 end 1 --> 2 & 3The previous version of
nu_outwas able to handle this case, by specifying that the incoming port in3is a boundary output port, whereas the incoming port in2is not. The new version is not, because the output boundary is now the outgoing port in1. Is the proof broken? No!! In fact, whilst the mapsnu_inpandnu_outcan express this case, thesubgraphfield ofSimpleReplacementcannot represent this subgraph (it is "non-convex", according to our definition of convexity). So this PR is in no way a regression in functionality. I will open a PR to handle this case inSiblingSubgraph, at which point we can updateSimpleReplacementto handle it too.BREAKING CHANGE:
SimpleReplacement::newwas replaced bySimpleReplacement::try_new.SimpleReplacement::{map_host_output, map_replacement_input}take and return opposite port directions.