-
Notifications
You must be signed in to change notification settings - Fork 11
fix: Encoding of opaque subgraphs with no associated qubit/bit #1295
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
Changes from 6 commits
ba4bae3
3f30aa9
de0c2e3
8fa9ef0
32ba9a9
689b222
54c9a9f
3168fef
ba82b8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,20 +75,28 @@ pub(super) struct EncodedCircuitInfo { | |
| /// pytket circuit, as they cannot be attached to a pytket command. | ||
| #[derive(Debug, Clone)] | ||
| pub(super) struct AdditionalNodesAndWires { | ||
| /// A subgraph of the region that does not contain any operation encodable | ||
| /// as a pytket command, and has no qubit/bits in its boundary that could be | ||
| /// used to emit an opaque barrier command in the [`serial_circuit`]. | ||
| pub extra_subgraph: Option<SubgraphId>, | ||
| /// Parameter expression inputs to the `extra_subgraph`. | ||
| /// These cannot be encoded either if there's no pytket command to attach them to. | ||
| pub extra_subgraph_params: Vec<String>, | ||
| /// Subgraphs of the region that could not be encoded as a pytket commands, | ||
| /// and have no qubit/bits in their boundary that could be used to emit an | ||
| /// opaque barrier command in the [`serial_circuit`]. | ||
| pub additional_subgraphs: Vec<AdditionalSubgraph>, | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The subgraphs we extract are always convex. We could technically remove that invariant and merge different subgraphs together, but at the end of the day they are just sets of nodes so we don't win anything from doing that (And it complicates the logic of ID tracking).
This is using the exact same behaviour we use for barriers; This produces convex subgraphs (due to the toposort traversal of nodes), but may produce multiple subgraphs in some cases. E.g, if we have graph LR
subgraph 0 ["(0) Module"]
direction LR
subgraph 1 ["(1) [**FuncDefn: #quot;test#quot;**]"]
direction LR
style 1 stroke:#832561,stroke-width:3px
16["(16) Unsupported 1"]
17["(17) Unsupported 2"]
18["(18) Supported 1"]
19["(19) Supported 2"]
16-->18
17-->19
16-->17
end
end
and we extract nodes in order This doesn't really worsen anything. We'll just have two barriers / two external subgraphs instead of one.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, didn't realize that the algorithm only took nodes already found and marked unsupported in the traversal but not yet put in a subgraph. So the alternative algorithm of (1) partitioning nodes into two sets, and (2) trying to build maximal convex subgraphs from the unsupported nodes, would give fewer barriers, including at most one initial subgraph....but ok, this is no big deal. Might be worth mentioning in a comment that all elements in the Vec here could have been combined into one subgraph in case we ever come back to this (I don't think it's worth raising an issue!). (That is necessarily true, right? Any unsupported op after a supported one will become a barrier, not part of the additional/initial block...right?)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, if it comes after then it is connected by a bit/qubit so it will appear as a barrier inline. |
||
| /// List of wires that directly connected the input node to the output node in the encoded region, | ||
| /// and were not encoded in [`serial_circuit`]. | ||
| /// | ||
| /// We just store the input nodes's output port and output node's input port here. | ||
| pub straight_through_wires: Vec<StraightThroughWire>, | ||
| } | ||
|
|
||
| /// A subgraph of the encoded circuit that could not be associated to any qubit or bit register in the pytket circuit. | ||
| #[derive(Debug, Clone)] | ||
| pub(super) struct AdditionalSubgraph { | ||
| /// The subgraph of the region that could not be encoded as a pytket command, | ||
| /// and has no qubit/bits in its boundary that could be used to emit an opaque | ||
| /// barrier command in the [`serial_circuit`]. | ||
| pub id: SubgraphId, | ||
| /// Parameter expression inputs to the `subgraph`. | ||
| pub params: Vec<String>, | ||
| } | ||
|
|
||
| /// A wire stored in the [`EncodedCircuitInfo`] that directly connected the | ||
| /// input node to the output node in the encoded region, and was not encoded in | ||
| /// the pytket circuit. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,11 +6,13 @@ use std::sync::Arc; | |
| use hugr::builder::Container; | ||
| use hugr::hugr::hugrmut::{HugrMut, InsertedForest}; | ||
| use hugr::ops::{OpTag, OpTrait}; | ||
| use hugr::std_extensions::arithmetic::float_types::float64_type; | ||
| use hugr::types::Type; | ||
| use hugr::{Hugr, HugrView, Node, OutgoingPort, PortIndex, Wire}; | ||
| use hugr_core::hugr::internal::HugrMutInternals; | ||
| use itertools::Itertools; | ||
|
|
||
| use crate::extension::rotation::rotation_type; | ||
| use crate::serialize::pytket::decoder::{ | ||
| DecodeStatus, FoundWire, LoadedParameter, PytketDecoderContext, TrackedBit, TrackedQubit, | ||
| }; | ||
|
|
@@ -81,7 +83,7 @@ impl<'h> PytketDecoderContext<'h> { | |
| subgraph, qubits, bits, params, old_parent, new_parent, | ||
| )?; | ||
|
|
||
| self.rewire_external_subgraph_outputs(subgraph, qubits, bits, old_parent, new_parent)?; | ||
| self.rewire_external_subgraph_outputs(id, subgraph, qubits, bits, old_parent, new_parent)?; | ||
|
|
||
| Ok(DecodeStatus::Success) | ||
| } | ||
|
|
@@ -159,6 +161,7 @@ impl<'h> PytketDecoderContext<'h> { | |
| /// Helper for [`Self::insert_external_subgraph`]. | ||
| fn rewire_external_subgraph_outputs( | ||
| &mut self, | ||
| id: SubgraphId, | ||
| subgraph: &OpaqueSubgraph<Node>, | ||
| qubits: &[TrackedQubit], | ||
| bits: &[TrackedBit], | ||
|
|
@@ -171,19 +174,19 @@ impl<'h> PytketDecoderContext<'h> { | |
| let mut output_qubits = qubits; | ||
| let mut output_bits = bits; | ||
|
|
||
| for (ty, (src, src_port)) in subgraph | ||
| for ((out_idx, ty), (src, src_port)) in subgraph | ||
| .signature() | ||
| .output() | ||
| .iter() | ||
| .enumerate() | ||
| .zip_eq(subgraph.outgoing_ports()) | ||
| { | ||
| // Output wire from the subgraph. Depending on the type, we may need | ||
| // to track new qubits and bits, re-connect it to some output, or | ||
| // leave it untouched. | ||
| let wire = Wire::new(*src, *src_port); | ||
| if let Some(counts) = self.config().type_to_pytket(ty).filter(|c| c.params == 0) { | ||
| // This port declares new outputs to be tracked by the decoder. | ||
| // Output parameters from a subgraph are always marked as not supported (they don't map to any pytket argument variable). | ||
| // This port declares new bit/qubit outputs to be tracked by the decoder. | ||
|
|
||
| // Make sure to disconnect the old wire. | ||
| self.builder.hugr_mut().disconnect(*src, *src_port); | ||
|
|
@@ -204,6 +207,15 @@ impl<'h> PytketDecoderContext<'h> { | |
| wire_qubits.unwrap().iter().cloned(), | ||
| wire_bits.unwrap().iter().cloned(), | ||
| )?; | ||
| } else if [rotation_type(), float64_type()].contains(ty) { | ||
|
||
| let param_name = id.output_parameter(out_idx); | ||
| let param = if ty == &rotation_type() { | ||
| LoadedParameter::rotation(wire) | ||
| } else { | ||
| LoadedParameter::float_half_turns(wire) | ||
| }; | ||
| self.wire_tracker | ||
| .register_input_parameter(param, param_name)?; | ||
| } else { | ||
| // This is an unsupported wire. If it was connected to the old | ||
| // region's output, rewire it to the new region's output. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive-by bump so I don't have to write a workaround for derive_more#454
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon I've gone to some massive effort to avoid this in some of my other PR(/attempt)s....great to see that! :)