feat: add new Subcircuit #1289

acl-cqc · 2025-11-28T15:35:57Z

Part two of Luca's #1273, following #1288.

I've dropped the as_const_value, as_const_f64 and get_const_inputs methods for now as they are only used in a test, so all that will be in the third part

codecov · 2025-11-28T15:39:21Z

Codecov Report

❌ Patch coverage is 75.06329% with 197 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.76%. Comparing base (c0d1236) to head (4aa5db0).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
tket/src/subcircuit.rs	75.85%	155 Missing and 14 partials ⚠️
tket/src/rewrite/strategy.rs	0.00%	17 Missing ⚠️
tket/src/resource/scope.rs	85.71%	5 Missing and 2 partials ⚠️
tket/src/resource/types.rs	0.00%	2 Missing ⚠️
tket/src/rewrite/trace.rs	0.00%	1 Missing ⚠️
tket/src/subcircuit/interval.rs	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1289      +/-   ##
==========================================
- Coverage   79.53%   78.76%   -0.77%     
==========================================
  Files         160      161       +1     
  Lines       20571    21293     +722     
  Branches    19605    20327     +722     
==========================================
+ Hits        16361    16772     +411     
- Misses       3227     3527     +300     
- Partials      983      994      +11

Flag	Coverage Δ
python	`92.92% <ø> (ø)`
qis-compiler	`100.00% <ø> (ø)`
rust	`78.07% <75.06%> (-0.78%)`	⬇️

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.

acl-cqc

Haven't quite followed all of this but most of it seems good, @lmondada I am proposing that I can do the alterations if you thumbs up / down my comments? Tho there are one or two that I haven't understood i.e. questions that perhaps you can answer!

acl-cqc · 2025-11-28T15:48:14Z

tket/src/resource.rs

 // Public API exports
 pub use flow::{DefaultResourceFlow, ResourceFlow, UnsupportedOp};
-pub use scope::{ResourceScope, ResourceScopeConfig};
+pub use scope::ResourceScope;


Is there good reason to drop this public export?

No, feel free to leave it :)

acl-cqc · 2025-11-28T15:51:04Z

tket/src/resource/scope.rs

    }

-    /// Get the port of node on the given resource path.
+    /// Get the ports of node with the given opvalue in the given direction.


with the given [CircuitUnit], I guess, but I'm more puzzled - why might there be more than one?

get_resource_port handles the linear case, and limits to at-most-one.

So I presume this means you might have multiple classical ports with the same unit....ah, if you are asking for inputs, and multiple inputs are wired from the same classical port? (But it'll never be >1 for Direction::Outgoing?)

Is that right? If so I can just add a comment

Yes, that is exactly the point. For copyable types, you may have more than one incoming port connected to the same value (identified by (Node, OutgoingPort))

acl-cqc · 2025-11-28T15:51:33Z

tket/src/resource/scope.rs

+            .expect("linear resource")
+    }
+
+    /// Get the resource ID at the given port of the given node.


Note to self - so this has just moved

acl-cqc · 2025-11-28T15:54:30Z

tket/src/resource/types.rs

    /// Create a new ResourceId.
    ///
-    /// This method should only be called by ResourceAllocator and tests.
+    /// ResourceIds should typically be obtained from [`ResourceAllocator`].


I'll put this in some #[cfg(test)]

acl-cqc · 2025-11-28T15:55:50Z

tket/src/resource/scope.rs


    /// All copyable wires on the ports of `node` in the given direction.
-    pub fn get_copyable_wires(
+    pub fn all_copyable_wires(


I note this is called only once, with Direction::Outgoing, for which case it is exactly the same as get_copyable_ports.

Whereas all_copyable_wires(...., Direction::Incoming), which is never used, there is nothing to tell you which port of node the specified Wire is connected to...

So I suggest either dropping this one (have just get_copyable_ports), or have two methods, one for inputs returning (IncomingPort, Wire)s and one for outputs returning just Wires.

(There is only one case where you pass a non-constant direction to get_copyable_ports and I am minded to rewrite that...)

Yes good spot, this is redundant. I agree that get_copyable_ports is sufficient.

acl-cqc · 2025-11-28T18:08:25Z

tket/src/subcircuit.rs

+    if other_inputs.iter().flatten().any(|(n, p)| {
+        circuit
+            .get_circuit_unit(*n, *p)
+            .is_none_or(|u| u.is_resource())


Do you mean is_some_and here?

Otherwise we are just gonna error if any of them is_none...that might be what we want?

No, I think this is correct: None would mean that the port is not valid (or that there is something wrong with circuit). The error could be refined to "resource or unknown input found after copyable inputs", or something like that. Technically, you could even panic if get_circuit_unit is None, because that means we have an invalid port somehow...

acl-cqc · 2025-11-28T18:09:07Z

tket/src/subcircuit.rs

+        .peeking_take_while(|&(node, port)| {
+            circuit
+                .get_circuit_unit(node, port)
+                .is_some_and(|unit| unit.is_resource())


here and elsewhere - change circ.get_circuit_unit(n, p).is_some_and(|unit| unit.is_resource()) to circ.get_resource_id(n,p),is_some() ?

yes, it's a bit shorter, so might be a good idea. I find the current version quite readable, but choose the one that you prefer :)

acl-cqc · 2025-11-28T18:13:45Z

tket/src/subcircuit.rs

+        let mut subcircuit = Self::try_from_resource_nodes(resource_nodes, circuit)?;
+
+        // Now adjust the boundary of subcircuit to match the subgraph
+        let (resource_inputs, copyable_inputs) = parse_input_boundary(subgraph, circuit)?;


hang on - do we expect linears to precede copyables for an arbitrary SiblingSubgraph??

Yes -- parse_input_boundary will return an error if that is not the case. This is for round-tripping: any SiblingSubgraph that is constructed from a Subcircuit will be of this form. By only accepting SiblingSubgraphs that satisfy this constraint, we make sure we never shuffle the boundary and thus surprise the user.

Admittedly this behaviour must be added to the docstring!! Good spot, thank you :)

Note that it would be easy to add a method to SiblingSubgraph to explicitly shuffle the boundary to satisfy this condition.

acl-cqc · 2025-11-28T18:14:35Z

tket/src/subcircuit.rs

+            .collect_vec();
+        for (node, port) in remove_inputs {
+            subcircuit
+                .try_remove_copyable_input(node, port, circuit)


Gonna think about this. I'm worried some removals might fail because you haven't done the additions yet / in the right order or something like that.

I think the comment I added at the definition site of try_remove_copyable_input should help. This is safe because all ports in remove_inputs should be covered by other inputs (added when subcircuit.input_copyable was extended above). These other ports are not removed (and would fail to be removed if attempted). So the order in which these "redundant" (covered) inputs are removed is not important.

acl-cqc · 2025-11-28T18:18:20Z

tket/src/subcircuit.rs

+    ///
+    /// This is possible when the input can be expressed as a function of other
+    /// inputs or computations within the subcircuit.
+    pub fn try_remove_copyable_input(


This function is pretty tough. Haven't got my head round it yet (apologies, I think I'm still less than 100% well)...I'm puzzled how any input was an input if it can be expressed as a function of the other inputs - that sounds nonconvex? I would think removing an input would require either

extending the subgraph outwards (to incorporate the node computing the old/removed input, thus gaining that new node's inputs, and also removing any other inputs that were outputs of the same new node)

shrinking the subgraph inwards, i.e. we lose the nodes that consumed that the old/removed input, and add inputs equal to the outputs of the nodes that were lost

However, I think you may be addressing something much more of a special case, in which case - do we want this to be pub?

Your intuition is correct. More specifically, the behaviour here is close to your first bullet point. Let me explain.

First of all, note that this only makes sense for classical inputs. In that case, it might be that a port is an input AND the input P is "covered" by other inputs, i.e. any path from a node outside of the subcircuit to P contains another input port (in the past of P).

This is the only case where removing the input P will be successful---and is what this method attempts to do.

Should this be public? Probably not, as I would not expect that users would have to modify the boundary with this level of detail. At the same time, it should never do something invalid, so the user shouldn't be able to "shoot themselves in the foot" if they used it.

Would it be helpful if I rewrote this function, with more comments and possibly breaking some stuff into helper functions?

acl-cqc · 2025-11-30T13:23:22Z

tket/src/subcircuit.rs

+#[derive(Debug, Clone, PartialEq)]
+pub struct Subcircuit<N: HugrNode = hugr::Node> {
+    /// The resource intervals making up the resource part of the subcircuit
+    intervals: Vec<Interval<N>>,


We need the ordering, right - we can't change this to a Map<ResourceId, Interval>?

The ordering defines the order of the boundary in SiblingSubgraph, and ultimate the rewrites. It is important because circuit rewrites map the i-th input/output of the subgraph to the i-th input/output of the replacement circuit.

That being said, you could use an IndexMap? That would both preserve the ordering as well as being more explicit about the map relationship.

acl-cqc · 2025-11-30T13:24:58Z

tket/src/subcircuit.rs

+    intervals: Vec<Interval<N>>,
+    /// The subcircuit inputs that are copyable values.
+    ///
+    /// The copyable expressions within the subcircuit can be computed on the


I love this! 👍 👍 Storing all the CopyableExprs would entail duplicating all the shared nodes, and indeed, storing all the nodes on the way to the output boundary. This is much better!

lmondada · 2025-12-03T13:33:28Z

Hi Alan! Thank you so much, I'll have a look and answer tomorrow, hope that's okay!! Sorry for the delay

…bcircuit

lmondada

Hey Alan!

Thank you so much for your work!! I think I should have answered all your comments and help clarify things where it was unclear.

Note that I cannot "apply" any of your change suggestions, as I only have read access to the repo. So please go ahead and apply them yourself.

Do you have a clear idea how you want to proceed? If you estimate that making the changes required to this PR is a lot of work, I'm happy to pick up a part of that. Just let me know exactly what you'd like me to fix.

Ping me here or in email whenever!

lmondada · 2025-12-08T18:53:50Z

tket/src/resource/scope.rs


    /// All copyable wires on the ports of `node` in the given direction.
-    pub fn get_copyable_wires(
+    pub fn all_copyable_wires(


Yes good spot, this is redundant. I agree that get_copyable_ports is sufficient.

lmondada · 2025-12-08T18:54:55Z

tket/src/subcircuit.rs

+                .get_copyable_ports(n, Direction::Incoming)
+                .map(|p| p.as_incoming().expect("incoming port"))
+            {
+                let Some(expr) = self.get_copyable_expression(n, p, circuit) else {


looks good! I don't think I can apply the change somehow, so you go ahead :)

lmondada · 2025-12-08T18:55:19Z

tket/src/subcircuit.rs

+        let mut was_changed = false;
+
+        for dir in Direction::BOTH {
+            let copyable_ports = circuit.get_copyable_ports(node, dir);


Thank you!!

lmondada · 2025-12-08T18:56:24Z

tket/src/subcircuit.rs

+
+impl<H: HugrView> ResourceScope<H> {
+    fn is_convex(&self, _subcircuit: &Subcircuit<H::Node>) -> bool {
+        unimplemented!("is_convex is not yet implemented")


yes, absolutely. I will submit a PR with the implementation of this next! See also the deactivated tests (that will be reactivated asap)

lmondada · 2025-12-10T07:14:31Z

tket/src/subcircuit.rs

+/// a [`SiblingSubgraph`] needs to be constructed (see
+/// [`Subcircuit::try_to_subgraph`] and [`Subcircuit::validate_subgraph`]).
+///
+/// [`Subcircuit`] distinguishes between "pure copyable" nodes, which has


yes, please apply :)

lmondada · 2025-12-11T18:43:19Z

tket/src/subcircuit.rs

+    subgraph: &SiblingSubgraph<N>,
+    circuit: &ResourceScope<impl HugrView<Node = N>>,
+) -> Result<(Vec<(N, IncomingPort)>, IncomingPorts<N>), InvalidSubcircuit<N>> {
+    let mut inp_iter = subgraph.incoming_ports().iter().peekable();


Yes, you're probably right. I didn't try as I wasn't convinced it was worth the effort, but if you have an idea how to do it, please go ahead!

lmondada · 2025-12-11T18:44:54Z

tket/src/subcircuit.rs

+    Ok((resource_inputs, other_inputs))
+}
+
+fn parse_output_boundary<N: HugrNode>(


Yes, that's correct!

lmondada · 2025-12-11T18:46:30Z

tket/src/subcircuit.rs

+        .peeking_take_while(|&(node, port)| {
+            circuit
+                .get_circuit_unit(node, port)
+                .is_some_and(|unit| unit.is_resource())


yes, it's a bit shorter, so might be a good idea. I find the current version quite readable, but choose the one that you prefer :)

lmondada · 2025-12-11T18:51:34Z

tket/src/subcircuit.rs

+}
+
+fn extend_unique<E: PartialEq>(vec: &mut Vec<E>, other: impl IntoIterator<Item = E>) {
+    let other_unique = other.into_iter().filter(|x| !vec.contains(x)).collect_vec();


You could replace the vector of copyable inputs/outputs with an IndexSet? That should preserve ordering and have cheap dedup.

That being said, I expect most subcircuits to have <10 classical inputs/outputs, so I was not worried about the cost here. The asymptotic would only ever be an issue if we have huge subcircuits in the future...

lmondada · 2025-12-11T18:52:40Z

tket/src/subcircuit.rs

+    let mut out_iter = subgraph.outgoing_ports().iter().copied().peekable();
+
+    let resource_outputs = out_iter
+        .peeking_take_while(|&(node, port)| {


I like that plan

lmondada and others added 11 commits November 23, 2025 21:11

Replace Subcircuit with SiblingSubgraph

1471c0b

Revert import change

159af20

Do not parametrize Rewriter, that's next

9d543f0

Fix strategy tests with TEMP TEST WORKAROUND

17f2870

Don't generalize code in CircuitRewrite (yet)

c17d23f

Don't need to polymorphize CircuitRewrite->RewriteTrace either

b6add8e

Simplify TupleUnpack

47d167e

Simplify TEMP in strategy.rs

f888d39

Generalize CircuitRewrite (inc. conversion to RewriteTrace)

63ea8c8

Add new SubCircuit

6ca3c4a

Remove as_const_value, as_const_f64, get_const_inputs

fb7b5ad

acl-cqc mentioned this pull request Nov 28, 2025

feat: add methods to Subcircuit for finding const inputs #1290

Draft

acl-cqc mentioned this pull request Nov 28, 2025

feat!: Replace Subcircuit // take 4 #1273

Closed

acl-cqc changed the base branch from main to lm/rm_subcircuit November 28, 2025 15:44

acl-cqc commented Nov 28, 2025

View reviewed changes

acl-cqc commented Nov 30, 2025

View reviewed changes

Base automatically changed from lm/rm_subcircuit to main December 3, 2025 11:07

acl-cqc added 3 commits December 3, 2025 14:00

Merge commit 'c0d12360c5f9c1ed01c5c46513b8e1e4abbe5d89' into lm/rm_su…

23ff42f

…bcircuit

Merge branch 'lm/rm_subcircuit' into lm/new_subcircuit

474abdf

fmt

4aa5db0

lmondada reviewed Dec 11, 2025

View reviewed changes

feat: add new Subcircuit #1289

Are you sure you want to change the base?

feat: add new Subcircuit #1289

Conversation

acl-cqc commented Nov 28, 2025

Uh oh!

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

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acl-cqc Nov 28, 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

acl-cqc Nov 30, 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!

lmondada commented Dec 3, 2025

Uh oh!

lmondada 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!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 28, 2025 •

edited

Loading

acl-cqc Nov 28, 2025 •

edited

Loading

acl-cqc Nov 30, 2025 •

edited

Loading