feat!: pytket EncodedCircuit struct for in-place pytket optimisation #1211
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1211 +/- ##
==========================================
+ Coverage 78.77% 78.92% +0.14%
==========================================
Files 155 159 +4
Lines 19177 20157 +980
Branches 18075 19055 +980
==========================================
+ Hits 15107 15908 +801
- Misses 3112 3275 +163
- Partials 958 974 +16
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:
|
aborgna-q
changed the title
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Well, massive PR...but great work @aborgna-q, looks like it was worth it! :-)
Corresponding mass of comments, but to summarize
- A few issues in subgraph.rs where I'm not sure what you're trying to do with edges
- That
pending_encoded_edge_connectionsthing needs addressing but I think is straightforward to move into WireTracker (?)
- That
- Other than that, everything is minor, although it'd be nice to see
- Something nicer than the mutable
Option<PytketDecoderConfig>+expect - PytketDecoderContext::new() taking an Option (more like the encoder!!) rather than having a separate
fn register_opaque_subgraphs - OpaqueSubgraphs::new(encoder_count) instead using
Node::index()
- Something nicer than the mutable
I'm not very familiar with encoder.rs so assume that's all ok 😉 😆 and haven't looked at tests yet, but definitely feels like we are nearly there.
tket/src/serialize/pytket/circuit.rs
Outdated
| pub serial_circuit: SerialCircuit, | ||
| /// A subgraph of the region that does not contain any operation encodable | ||
| /// as a pytket command, and hence was not encoded in [`serial_circuit`]. | ||
| #[expect(unused)] |
There was a problem hiding this comment.
Is there a TODO associated here?
There was a problem hiding this comment.
How come only one....or can a SubgraphId reference something that's disconnected (and likely very nonconvex)?
There was a problem hiding this comment.
It's one of the commented-out failing tests with a TODO.
These are subgraphs that connect only to the input and output node (if anything), using non-pytket types.
There's at most one of these, since they can always be merged together (I think we can have two disconnected components in a subgraph? I'll change it into a vec if needed later)
There was a problem hiding this comment.
Ah ok. There could be multiple parallel such, in which case they could be merged; but not sequential - there'd have to be a break between them, i.e. a supported/encoded node, in which case....these nodes (between Input and SerialCircuit, or between SerialCircuit and Output) would be included in a "normal" unsupported subgraph? (Right?)
| /// [`EncodedCircuit::new_standalone`] or call | ||
| /// [`EncodedCircuit::ensure_standalone`]. | ||
| #[derive(Debug, Clone)] | ||
| pub struct EncodedCircuit<Node: HugrNode> { |
There was a problem hiding this comment.
Does this (not storing the Hugr/View in here) mean that the Hugr could be mutated independently?
There was a problem hiding this comment.
It could, yes.
This pattern is common in other "non-borrowing" structures though, like petgraph's Toposort: https://docs.rs/petgraph/latest/petgraph/visit/struct.Topo.html
| } | ||
|
|
||
| impl EncodedCircuit<Node> { | ||
| /// Encode a HugrView into a [`EncodedCircuit`]. |
There was a problem hiding this comment.
| /// Encode a HugrView into a [`EncodedCircuit`]. | |
| /// Encode a Hugr into a [`EncodedCircuit`]. |
I mean, Node=Node and AsRef and AsMut is close enough to Hugr, right. (But I'm a little surprised by the need for AsMut - presumably some trait needs this? I mean, you can't actually mutate through the &Circuit<H>, can you?)
There was a problem hiding this comment.
Yeah, AsMut is pretty much a Hugr.
The limitation are the Hugr builder traits, that require AsMut/AsRef. That's why decoding is only done on Hugrs.
| // connected once the outgoing port is created. | ||
| // | ||
| // This handles the case where unsupported subgraphs in opaque barriers on | ||
| // the pytket circuit get reordered and input ports are seen before their |
There was a problem hiding this comment.
Eeek. I assume that this cannot create a cycle of dataflow edges...
There was a problem hiding this comment.
It shouldn't happen normally... the user is free to modify their circuit and see invalid hugrs though.
I didn't focus to much on this usecase for this PR, we may be able to improve the UX for standalone extracting/re-encoding later.
There was a problem hiding this comment.
I think this should be ok? (Modification excepted.) You'd have to have two barriers, which are "parallel" from the pytket view (no qubits between them) so they could be reordered, but with an unsupported wire between them - in which case the unsupported subgraphs would be merged into a single barrier?
| qubit_args: &mut &[TrackedQubit], | ||
| bit_args: &mut &[TrackedBit], | ||
| params: &mut &[LoadedParameter], | ||
| unsupported_wire: Option<EncodedEdgeID>, |
There was a problem hiding this comment.
nit: it's a little surprising that the unsupported_wire here has no relation to self.unsupported_wires (at least within find_typed_wire)....the parametrization trick suggested elsewhere might make that more obvious, as would renaming self.unsupported_wires to self.pending_edge_connections
| // TODO: Test that unsupported subgraphs that don't affect any qubit/bit registers | ||
| // TODO: Test that opaque subgraphs that don't affect any qubit/bit registers | ||
| // are correctly encoded in pytket commands. | ||
| let mut extra_subgraph: Option<BTreeSet<H::Node>> = None; |
There was a problem hiding this comment.
I think this might be simpler just to use a non-Option BTreeSet and check whether it's empty
aborgna-q
force-pushed
the
ab/encoded-circuit
branch
from
780de1c to
76c8cef
Compare
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Great work, thanks @agustin. Particularly like the balance you've struck between bits that are the same between external/inline and bits that are different. Looks good to me, tho I think it's worth
- adding issues for some of the TODOs that don't work yet
- combining the IndexMaps (I think that's the only change I suggest here that's not trivial)
- please doc what
extra_subgraphis for ;)
The most controversial point is probably that you haven't stored an immutable reference to the Hugr in the encoded circuit, but I'm happy with that - I slightly laugh at (but like) your ExternalSubgraphWasModified error 😁. I'll admit to being wrong on a few of my suggestions before, too....
| /// A subgraph of the region that does not contain any operation encodable | ||
| /// as a pytket command, and hence was not encoded in [`serial_circuit`]. | ||
| #[expect(unused)] | ||
| pub extra_subgraph: Option<SubgraphId>, |
There was a problem hiding this comment.
This is now read :+1 but AFAICS never written?? I think the comment could be better - all unsupported subgraphs "do not contain any operation encodable as a pytket command" so is this for (a) if the entire region did not contain any pytket commands, or (b) if there were unsupported nodes that were disconnected from the pytket-encoded ones, or (c) something else ???
There was a problem hiding this comment.
This is set by EncoderContext::finish, when creating the EncodedCircuitInfo.
I extended the comment a bit.
…1224) Depends on #1211 These are not represented in the pytket circuit, and cannot be encoded into a SiblingSubgraph. We just track them as an additional item in the `EncodedCircuit`'s `EncodedCircuitInfo`. Note that the info is only store for circuits we encode directly, and not for nested regions inside circuit boxes. This means that the info is lost when encoding those. I added a commented-out test with a TODO to fix that (we'll need some extra plumbing to match circ boxes to external metadata). drive-by: Make sure the encoder's WireTracker stores the input parameter names, and passes it along. I'm ignoring breaking changes since they only affect unpublished code.
For previous reviews, see
Comments from those have ben addressed already.
This PR adds an
EncoderCircuitin between the encoding/decoding processHugr <-> tket_json_rs::SerialCircuit.This new instruction carries multiple
SerialCircuitsassociated with the Hugr region they encode. This lets us replace the old regions with modified ones in the original hugr, after optimising the SerialCircuits.The opaque barriers that represent sections of the original hugr not encodable as pytket commands now have two payload variants:
OpaqueSubgraphPayload::Inlineas before encodes the unsupported hugr envelope, plus some boundary edge ids that we can use to reconstruct links between different inline payloads.This variant does not support all circuits. Subgraphs must be flat and contain no non-local edges (just as before, but now we detect and error out on those cases).
OpaqueSubgraphPayload::External. This references a subgraph in the original hugr with a simple ID. This allows us to re-use the nodes from the initial hugr, keeping all the hierarchy and edges.The second case only works for
Hugrs due toHugrBuilderlimitations, but it's the usecase we'll use for tket1 optimisation.The first one is kept for cases where we need to extract standalone pytket circuits.
For testing, this includes
serialize::pytket::tests, including error teststket1-passesthat encodes a flat hugr, applies a pass from pytket, and reassembles it inline.BREAKING CHANGE:
OpConvertErrorrenamed toPytketEncodeOpErrorBREAKING CHANGE: Some minor changes to
PytketDecodeErrorvariant attributes.BREAKING CHANGE:
fn_namemoved fromDecodeOptionstoDecodeInsertionTarget::Function