fix: Encoding of opaque subgraphs with no associated qubit/bit #1295

aborgna-q · 2025-12-02T16:54:54Z

This PR addresses a problem when emitting subgraphs that cannot be put in a pytket barrier since they were not associated with registers.

Given the following graph:

graph LR
    subgraph 0 ["(0) Module"]
        direction LR
        subgraph 1 ["(1) [**FuncDefn: #quot;test#quot;**]"]
            direction LR
            style 1 stroke:#832561,stroke-width:3px
            2["(2) Input"]
            3["(3) Output"]
            16["(16) Unsupported OP"]
            17["(17) tket.rotation.from_halfturns_unchecked"]
            16--"0:0<br>float64"-->17
            17--"0:0<br>rotation"-->3
        end
    end

The encoding of from_halfturns_unchecked would request the unsupported op to be emitted in an opaque subgraph, but we are unable to put that operation alone in a barrier.

While #1294 proposes doing a lookahead on the wires and backtracking/marking from_halfturns_unchecked as unsupported as well, the solution here is simpler.
We just let the EncodedCircuit metadata carry multiple subgraphs not linked to barriers, so we register them there.

The downside is that now the circuit above is not encodable as a standalone pytket circuit (since the metadata stays on the rust in-memory object), but we can change it in the future if needed.

aborgna-q · 2025-12-02T16:58:54Z

Cargo.toml

 csv = "1.4.0"
 delegate = "0.13.5"
-derive_more = "2.0.1"
+derive_more = "2.1.0"


Drive-by bump so I don't have to write a workaround for derive_more#454

I reckon I've gone to some massive effort to avoid this in some of my other PR(/attempt)s....great to see that! :)

codecov · 2025-12-02T16:59:18Z

Codecov Report

❌ Patch coverage is 92.15686% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.55%. Comparing base (35c3a1e) to head (ba82b8a).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
tket/src/serialize/pytket/decoder/subgraph.rs	81.81%	0 Missing and 2 partials ⚠️
tket/src/serialize/pytket/decoder.rs	88.88%	0 Missing and 1 partial ⚠️
tket/src/serialize/pytket/encoder.rs	96.15%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1295      +/-   ##
==========================================
+ Coverage   79.53%   79.55%   +0.02%     
==========================================
  Files         160      160              
  Lines       20571    20578       +7     
  Branches    19605    19612       +7     
==========================================
+ Hits        16361    16371      +10     
+ Misses       3227     3225       -2     
+ Partials      983      982       -1

Flag	Coverage Δ
python	`92.92% <ø> (ø)`
qis-compiler	`100.00% <ø> (ø)`
rust	`78.88% <92.15%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aborgna-q · 2025-12-02T18:31:57Z

Github seems to be returning null for merge_commit_sha, which causes the rs-semver-checks action to misbehave.

This happened once before in an incident, so it may be same this time?

Anyways, it's safe to ignore the bot diagnostics.

acl-cqc · 2025-12-03T11:00:20Z

We just let the EncodedCircuit metadata carry multiple subgraphs not linked to barriers

Does there ever need to be more than one? If there are no qubit wires between the "multiple subgraphs", that can just be one big subgraph, no? And if there are qubit wires, then the subgraphs to which those wires are input, become barriers?

acl-cqc

So this "fixes" the UnsupportedSubgraphHasNoRegisters error seen on optimize_flattened_guppy::case_2_angles in my #1270 by converting it to a NoMatchingParameter....

Generally seems like a plan, but I'd at least like to hear if you think (and why) we really need multiple additional-subgraphs. Combining them together seems very much like what we do with barriers ATM (?) and the tricky cases are also (only) the ones that happen with unsupported subgraphs midcircuit, AFAICS.

tket/src/serialize/pytket/encoder.rs

acl-cqc · 2025-12-03T11:12:15Z

tket/src/serialize/pytket/encoder.rs

-            return Err(PytketEncodeError::UnsupportedSubgraphHasNoRegisters {
+            // We cannot associate this subgraph to any qubit or bit register in the pytket circuit,
+            // so we'll store it in the [`AdditionalSubgraph`]s instead when finishing the encoding.
+            self.non_emitted_subgraphs.push(AdditionalSubgraph {


It might be as simple as, just add the nodes to the unique additional-subgraph, perhaps with some assert that the resulting subgraph is convex - obviously if that assert breaks then my supposition is false, but ATM I don't see how it can.

(A qubit wire directly from one unsupported bit to another is OK I guess, as long as there are no supported ops on said wire.)

We don't store the nodes here, but rather a subgraphID that's registered in the subgraph tracker.

Merging them would require adding some union-find thing to the tracker, or doing the backtracking lookahead I mentioned in the PR description.

I don't think it's really worth the effort/complexity for no real upsides.

Yeah ok, put a comment somewhere in case we ever decide reducing #barriers / merging these things becomes important, and leave it

acl-cqc · 2025-12-03T11:13:50Z

tket/src/serialize/pytket/tests.rs

+/// A program with an unsupported subgraph not associated to any qubit or bit.
+/// <https://github.com/CQCL/tket2/issues/1294>
+#[fixture]
+fn circ_unsupported_subgraph_no_registers() -> Circuit {


This test looks fine, but you should probably add one which (currently) requires multiple additional-subgraphs...challenge if you can come up with a case that can't be encoded as just one?

👍 Modifying this to follow the structure from my comment above.

Co-authored-by: Alan Lawrence <[email protected]>

aborgna-q · 2025-12-04T11:29:03Z

tket/src/serialize/pytket/circuit.rs

+    /// 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>,


Does there ever need to be more than one? If there are no qubit wires between the "multiple subgraphs", that can just be one big subgraph, no? And if there are qubit wires, then the subgraphs to which those wires are input, become barriers?

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).

Combining them together seems very much like what we do with barriers ATM

This is using the exact same behaviour we use for barriers;
when we require the register/param in an edge coming from an unsupported op we do a greedy BFS extracting all connected not-yet-extracted nodes we have seen.

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

Loading

and we extract nodes in order 16,18,17,19, we'll create two subgraphs where one containing {16,17} would have sufficed.

This doesn't really worsen anything. We'll just have two barriers / two external subgraphs instead of one.

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?)

Yes, if it comes after then it is connected by a bit/qubit so it will appear as a barrier inline.

aborgna-q · 2025-12-04T11:41:07Z

tket/src/serialize/pytket/encoder.rs

-            return Err(PytketEncodeError::UnsupportedSubgraphHasNoRegisters {
+            // We cannot associate this subgraph to any qubit or bit register in the pytket circuit,
+            // so we'll store it in the [`AdditionalSubgraph`]s instead when finishing the encoding.
+            self.non_emitted_subgraphs.push(AdditionalSubgraph {


We don't store the nodes here, but rather a subgraphID that's registered in the subgraph tracker.

Merging them would require adding some union-find thing to the tracker, or doing the backtracking lookahead I mentioned in the PR description.

I don't think it's really worth the effort/complexity for no real upsides.

aborgna-q · 2025-12-04T11:41:24Z

tket/src/serialize/pytket/tests.rs

+/// A program with an unsupported subgraph not associated to any qubit or bit.
+/// <https://github.com/CQCL/tket2/issues/1294>
+#[fixture]
+fn circ_unsupported_subgraph_no_registers() -> Circuit {


👍 Modifying this to follow the structure from my comment above.

…bgraphs

…ames

acl-cqc

Looks good, thanks @aborgna-q, a few one-liner/nits is all I can see :)

acl-cqc · 2025-12-05T15:29:42Z

tket/src/serialize/pytket/decoder/subgraph.rs

                    wire_qubits.unwrap().iter().cloned(),
                    wire_bits.unwrap().iter().cloned(),
                )?;
+            } else if [rotation_type(), float64_type()].contains(ty) {


consider a static/constant group of these two...I think may be used elsewhere too. (It'd have to be a LazyStatic tho, so probably don't bother if this is the only place)

I think I got called out for prematurely optimizing this before :P

There's a couple cases in the decoder, I'll replace all with a single lazy lock.

tket/src/serialize/pytket/encoder.rs

acl-cqc · 2025-12-05T15:31:26Z

tket/src/serialize/pytket/encoder.rs

-            return Err(PytketEncodeError::UnsupportedSubgraphHasNoRegisters {
+            // We cannot associate this subgraph to any qubit or bit register in the pytket circuit,
+            // so we'll store it in the [`AdditionalSubgraph`]s instead when finishing the encoding.
+            self.non_emitted_subgraphs.push(AdditionalSubgraph {


Yeah ok, put a comment somewhere in case we ever decide reducing #barriers / merging these things becomes important, and leave it

acl-cqc · 2025-12-05T15:33:44Z

tket/src/serialize/pytket/encoder.rs

+        } else {
+            // If there are registers to which to attach, emit it as a barrier command.
+
+            // Create pytket operation, and add the subcircuit as hugr


I think payload is a reference to an external subgraphid here, not a hugr, right?

tket/src/serialize/pytket/tests.rs

…bgraphs

Fixes #1297 Depends on #1295 We now store the subgraph input parameters in the payload, since pytket may remove values from the `params` field of a barrier command. BREAKING CHANGE: Added input_parameter fields to `OpaqueSubgraphPayload`

fix: Encoding of opaque subgraphs with no associated qubit/bit

ba4bae3

aborgna-q requested a review from a team as a code owner December 2, 2025 16:54

aborgna-q requested a review from acl-cqc December 2, 2025 16:54

aborgna-q commented Dec 2, 2025

View reviewed changes

acl-cqc reviewed Dec 3, 2025

View reviewed changes

Apply suggestions from code review

3f30aa9

Co-authored-by: Alan Lawrence <[email protected]>

aborgna-q commented Dec 4, 2025

View reviewed changes

Multiple subgraphs in test case

de0c2e3

aborgna-q requested a review from acl-cqc December 4, 2025 11:47

aborgna-q added 3 commits December 4, 2025 12:01

Merge remote-tracking branch 'origin/main' into ab/fix-unconnected-su…

8fa9ef0

…bgraphs

drive-by: subgraph output parameters should have valid pytket param n…

32ba9a9

…ames

fix: Register extra subgraph outputs _before_ doing the command decoding

689b222

aborgna-q force-pushed the ab/fix-unconnected-subgraphs branch from 46fb309 to 689b222 Compare December 5, 2025 13:07

aborgna-q mentioned this pull request Dec 5, 2025

fix: Don't rely on command params for pytket barriers #1298

Merged

acl-cqc approved these changes Dec 5, 2025

View reviewed changes

aborgna-q added 3 commits December 8, 2025 13:46

lazy lock with PARAMETER_TYPES

54c9a9f

review comments

3168fef

Merge remote-tracking branch 'origin/main' into ab/fix-unconnected-su…

ba82b8a

…bgraphs

aborgna-q added this pull request to the merge queue Dec 8, 2025

Merged via the queue into main with commit 0db6426 Dec 8, 2025
24 checks passed

aborgna-q deleted the ab/fix-unconnected-subgraphs branch December 8, 2025 14:32

hugrbot mentioned this pull request Dec 8, 2025

chore: release #1189

Draft

aborgna-q mentioned this pull request Dec 8, 2025

feat: NormalizeGuppy uses inline_acyclic, fixes 2*opt tests #1270

Open

fix: Encoding of opaque subgraphs with no associated qubit/bit #1295

fix: Encoding of opaque subgraphs with no associated qubit/bit #1295

Conversation

aborgna-q commented Dec 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aborgna-q commented Dec 2, 2025

Uh oh!

acl-cqc commented Dec 3, 2025

Uh oh!

acl-cqc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aborgna-q Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aborgna-q Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acl-cqc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 2, 2025 •

edited

Loading

acl-cqc left a comment •

edited

Loading

aborgna-q Dec 4, 2025 •

edited

Loading

aborgna-q Dec 4, 2025 •

edited

Loading