Directly use PackedOperation for two qubit decomposers #14650

mtreinish · 2025-06-21T01:00:19Z

Summary

This commit reworks the rust code for the two qubit decomposers to natively work with PackedOperation instead of doing Option<StandardGate> to represent either a StadandardGate::CX gate for cx based decomposition and None for any other gate used for the 2q target. Prior to having a more complete circuit data model defined in rust the two qubit decomposers were written using string identifiers where 1q gates were represented by their names and 2q gates were either CX or a sentinel value USER_GATE. This was because the TwoQubitBasisDecomposer has a custom code path that is CX only, but the other KAK gates used in the decomposer are generic. The USER_GATE string was used to tell the python code which was constructing the circuit from a gate sequence to use the gate instance stored in the python class instead of something hard coded. This enabled standard gates and custom gates.

However, since the two qubit decomposers were originally ported to Rust the Rust side of Qiskit has grown significantly and we can now natively build the circuit fully in Rust and represent all the operations. The model used in the decomposers was then expanded to work with Rust native gate representations but the same pattern using the sentinel value and custom operation reconstruction in Python was still used. This mismatch has caused confusion like as reported in #14418 but also hinders the usage of the decomposers in other rust callers. For example, looking at implementing #12213 we will need to work with the two qubit decomposer directly in rust for the qsd implementation and working with gates natively in rust will be necessary.

This commit migrates the custom sequence type used for the two qubit decomposer to work natively with a PackedOperation and store all operations used in the sequence. We can consider removing the custom sequence type once the last python usage from CircuitData is removed and we don't need the py token for interacting with global phase. It's slighly heavier weight than the custom sequence type, but we are double allocating and it will be simpler if everything is natively in CircuitData eventually. But for this commit it was better to keep the sequence type around and simplify it so the conversion is more straightforward.

Details and comments

Fixes #14418

This commit reworks the rust code for the two qubit decomposers to natively work with PackedOperation instead of doing `Option<StandardGate>` to represent either a StadandardGate::CX gate for cx based decomposition and `None` for any other gate used for the 2q target. Prior to having a more complete circuit data model defined in rust the two qubit decomposers were written using string identifiers where 1q gates were represented by their names and 2q gates were either CX or a sentinel value USER_GATE. This was because the TwoQubitBasisDecomposer has a custom code path that is CX only, but the other KAK gates used in the decomposer are generic. The USER_GATE string was used to tell the python code which was constructing the circuit from a gate sequence to use the gate instance stored in the python class instead of something hard coded. This enabled standard gates and custom gates. However, since the two qubit decomposers were originally ported to Rust the Rust side of Qiskit has grown significantly and we can now natively build the circuit fully in Rust and represent all the operations. The model used in the decomposers was then expanded to work with Rust native gate representations but the same pattern using the sentinel value and custom operation reconstruction in Python was still used. This mismatch has caused confusion like as reported in Qiskit#14418 but also hinders the usage of the decomposers in other rust callers. For example, looking at implementing Qiskit#12213 we will need to work with the two qubit decomposer directly in rust for the qsd implementation and working with gates natively in rust will be necessary. This commit migrates the custom sequene type used for the two qubit decomposer to work natively with a PackedOperation and store all operations used in the sequence. We can consider removing the custom sequence type once the last python usage from CircuitData is removed and we don't need the py token for interacting with global phase. It's slighly heavier weight than the custom sequence type, but we are double allocating and it will be simpler if everything is natively in CircuitData eventually. But for this commit it was better to keep the sequence type around and simplify it so the conversion is more straightforward. Fixes Qiskit#14418

qiskit-bot · 2025-06-21T01:00:23Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core
@levbishop

coveralls · 2025-06-21T01:33:20Z

Pull Request Test Coverage Report for Build 16292604447

Details

251 of 274 (91.61%) changed or added relevant lines in 5 files are covered.
16 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.009%) to 87.764%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/transpiler/src/passes/unitary_synthesis.rs	17	20	85.0%
crates/synthesis/src/two_qubit_decompose.rs	223	243	91.77%

Files with Coverage Reduction	New Missed Lines	%
crates/synthesis/src/two_qubit_decompose.rs	1	91.1%
crates/transpiler/src/passes/unitary_synthesis.rs	2	93.23%
crates/qasm2/src/lex.rs	3	91.75%
crates/circuit/src/symbol_expr.rs	4	73.86%
crates/qasm2/src/parse.rs	6	97.09%

Totals
Change from base Build 16290740978:	0.009%
Covered Lines:	81474
Relevant Lines:	92833

💛 - Coveralls

velocityraptor7085

Thank you @mtreinish. These changes look great. I have tried to leave some minor comments wherever possible.

crates/synthesis/src/two_qubit_decompose.rs

velocityraptor7085 · 2025-06-23T20:18:49Z

crates/transpiler/src/passes/high_level_synthesis.rs

                        2,
+                        0,


I'm unsure as to whether these would be considered magic numbers and could use some associated variable names (at least in this file, rather than in two_qubit_decompose.rs) or if they are already considered self-explanatory. I'll leave it up to your judgement @mtreinish.

I would consider these self explanatory because the from_packed_operation function's arguments document these as num_qubits and num_clbits for the circuit. So we're just passing the size of the circuit to the generator function. We're creating a 2 qubit 0 clbit circuit with this function call.

crates/synthesis/src/two_qubit_decompose.rs

…tBasisDecomposer

velocityraptor7085

The code looks good to me 🙂. Thank you @mtreinish.

kevinhartman

Adding just a couple of comments (and acknowledgement that this will be a fun merge for me with the control flow PR lol).

kevinhartman · 2025-06-24T19:21:18Z

crates/synthesis/src/two_qubit_decompose.rs

@@ -1268,13 +1264,11 @@ impl TwoQubitWeylDecomposition {
    }
 }

-type TwoQubitSequenceVec = Vec<(Option<StandardGate>, SmallVec<[f64; 3]>, SmallVec<[u8; 2]>)>;
+type TwoQubitSequenceVec = Vec<(PackedOperation, SmallVec<[f64; 3]>, SmallVec<[u8; 2]>)>;


Beyond the scope of this PR, but generally I think we should get out of the habit of having these alias types. It seems like it leads to an extra layer of indirection when trying to understand the code. I'd rather see a struct type with named fields for the element (PackedOperation, SmallVec<[f64; 3]>, SmallVec<[u8; 2]>) and then the Vec be inlined where applicable.

crates/synthesis/src/two_qubit_decompose.rs

Co-authored-by: Kevin Hartman <kevin@hart.mn>

kevinhartman

Before I finish reviewing, I think that PackedOperation isn't an ideal type for us here since it represents more than just a gate-like operation.

From the comment in this review pass, perhaps should we consider a new Gate enum to represent this:

enum Gate {
    Standard(StandardGate),
    Python(PyGate),
    Unitary(Unitary),
}

If you opt to do that, we could keep it local to this pass for now, but it might be a useful type to live somewhere in operations.rs at some point.

I wouldn't suggest this if you weren't already dealing directly with operations and f64 parameter vectors (as opposed to working with &PackedInstructions).

kevinhartman · 2025-07-03T18:21:21Z

crates/synthesis/src/two_qubit_decompose.rs

+                }
+                OperationRef::Gate(gate) => gate.gate.clone_ref(py),
+                OperationRef::Unitary(unitary) => unitary.create_py_op(py, None)?.into_any(),
+                _ => unreachable!("decomposer gate must be a gate"),


Despite the name of the PR, I think this is a good indicator that PackedOperation isn't the best type for us to use as TwoQubitDecomposer::gate. I think the right way to do this from a typing perspective would be to define a new enum that can be exactly one of the things we actually support, e.g.

enum Gate { Standard(StandardGate), Python(PyGate), Unitary(Unitary), }

I get where you're coming from here but I disagree. The point of this PR is to move all the processing to rust and move this closer to CircuitData with the goal of standardizing on that medium term. We're not at the point where that's feasible yet because of ParameterExpression but medium term this will all be replaced. Using a PackedOperation here will make that easier in the next steps.

I'm fine with using PackedOperation if it'll make things easier for us in the future. Though, I'm not sure I understand what you meant exactly by "move this closer to CircuitData". Could you elaborate?

Just what you said elsewhere, this is a step closer to using PackedInstruction. The eventual goal is to remove these types and just work natively with CircuitData instead of a custom vec type.

Ideally I'd like this to just return an iterator of PackedInstructions so we can collect it into either a CircuitData or DAGCircuit depending on the context calling this. But in the short term this is an incremental improvement working towards that.

crates/synthesis/src/two_qubit_decompose.rs

kevinhartman · 2025-07-07T20:47:43Z

crates/synthesis/src/two_qubit_decompose.rs

+                }
+                OperationRef::Gate(gate) => gate.gate.clone_ref(py),
+                OperationRef::Unitary(unitary) => unitary.create_py_op(py, None)?.into_any(),
+                _ => unreachable!("decomposer gate must be a gate"),


I'm fine with using PackedOperation if it'll make things easier for us in the future. Though, I'm not sure I understand what you meant exactly by "move this closer to CircuitData". Could you elaborate?

kevinhartman

Looks good, thanks for the changes!

mtreinish added this to the 2.2.0 milestone Jun 21, 2025

mtreinish added the Changelog: None Do not include in changelog label Jun 21, 2025

mtreinish requested a review from a team as a code owner June 21, 2025 01:00

mtreinish added synthesis Rust This PR or issue is related to Rust code in the repository labels Jun 21, 2025

mtreinish force-pushed the no-more-sentinel branch from aac39da to 6a408f2 Compare June 21, 2025 15:34

mtreinish mentioned this pull request Jun 21, 2025

Updated TwoQubitBasisDecomposer to use actual gate names instead of a sentinel string #14652

Open

velocityraptor7085 reviewed Jun 23, 2025

View reviewed changes

mtreinish added 4 commits June 24, 2025 03:24

Merge remote-tracking branch 'origin/main' into no-more-sentinel

92ae8f1

Add docstring to private methods on TwoQubitBasisDecomposer

f81f2d8

Add comment on 2q gate inversion

ff35587

Return an error instead of panicing on parameterized input to TwoQubi…

8b4aa6e

…tBasisDecomposer

mtreinish requested a review from velocityraptor7085 June 24, 2025 07:57

velocityraptor7085 approved these changes Jun 24, 2025

View reviewed changes

kevinhartman reviewed Jun 24, 2025

View reviewed changes

mtreinish and others added 5 commits July 2, 2025 07:10

Apply suggestions from code review

4da00af

Co-authored-by: Kevin Hartman <kevin@hart.mn>

Merge branch 'main' into no-more-sentinel

cba6e2f

Update private method docstrings

0c0fcc0

Use panic! instead of unreachable

4f1b1bb

Merge branch 'main' into no-more-sentinel

795cc97

mtreinish requested a review from kevinhartman July 2, 2025 12:17

raynelfss assigned mtreinish Jul 3, 2025

mtreinish assigned kevinhartman and unassigned mtreinish Jul 3, 2025

kevinhartman reviewed Jul 3, 2025

View reviewed changes

kevinhartman reviewed Jul 7, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into no-more-sentinel

0190be2

Use a constant for default capacity

5bef2e1

mtreinish requested a review from kevinhartman July 15, 2025 15:07

kevinhartman approved these changes Jul 16, 2025

View reviewed changes

kevinhartman added this pull request to the merge queue Jul 16, 2025

Merged via the queue into Qiskit:main with commit e9c4b47 Jul 16, 2025
26 checks passed

raynelfss added a commit to raynelfss/qiskit that referenced this pull request Jul 16, 2025

Fix: Changes introduced by Qiskit#14650

38c7030

Directly use PackedOperation for two qubit decomposers #14650

Directly use PackedOperation for two qubit decomposers #14650

Uh oh!

Conversation

mtreinish commented Jun 21, 2025

Summary

Details and comments

Uh oh!

qiskit-bot commented Jun 21, 2025

Uh oh!

coveralls commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16292604447

Details

💛 - Coveralls

Uh oh!

velocityraptor7085 left a comment

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!

velocityraptor7085 left a comment

Choose a reason for hiding this comment

Uh oh!

kevinhartman left a comment

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinhartman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coveralls commented Jun 21, 2025 •

edited

Loading