Remove duplicated method on TwoQubitBasisDecomposer by mtreinish · Pull Request #15835 · Qiskit/qiskit

mtreinish · 2026-03-19T01:33:29Z

Summary

This commit removes a duplicated method implementation on the
TwoQubitBasisDecomposer. The struct had two methods, call_inner and
generate_sequences, which were almost line for line identical. I believe
this occured during a rebase when two concurrently developed PRs were
touching the same file and the rebase was a bit off after one merged
which resulted in the methods being duplicated. This was missed for
quite some time and is what prompted #15833 because the original
two_qubit_decompose.rs file was so big it was very easy to overlook this
duplication. This commit deletes the generate_sequence implementation
and just uses the call_inner function.

Details and comments

~~This PR is based on top of #15833 and will need to be rebased after that merges. In the meantime you can view the contents of this PR by looking at the HEAD commit on this PR's branch: 73353ae~~ Rebased now

qiskit-bot · 2026-03-19T01:33:33Z

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

@Qiskit/terra-core
@levbishop

ShellyGarion

Copying here a few comments from #15833, and a few more comments about the reorganization of the files that may slightly be improved.
perhpas we can combine these small changes as part of this PR?

ShellyGarion · 2026-03-23T08:30:21Z

+
+const PI2: f64 = PI / 2.;
+const PI4: f64 = PI / 4.;
+const TWO_PI: f64 = 2.0 * PI;


perhaps it's worth to use the usual constants:
FRAC_PI_2, FRAC_PI_4, FRAC_2_PI

ShellyGarion · 2026-03-23T08:40:52Z

+    }
+}
+
+fn u4_to_su4(u4: ArrayView2<Complex64>) -> (Array2<Complex64>, f64) {


perhaps this function could also be moved to common?

ShellyGarion · 2026-03-23T08:42:16Z

+    (su4, phase_factor.arg())
+}
+
+fn real_trace_transform(mat: ArrayView2<Complex64>) -> Array2<Complex64> {


could you please add a docstring to this function?
perhaps this function could also be moved to common?

ShellyGarion · 2026-03-23T08:43:12Z

+    Ok((real_map, circ))
+}
+
+fn compute_unitary(sequence: &TwoQubitSequenceVec, global_phase: f64) -> Array2<Complex64> {


perhaps this function could also be moved to common?

ShellyGarion · 2026-03-23T08:44:25Z

+use super::gate_sequence::TwoQubitGateSequence;
+use super::weyl_decomposition::{Specialization, TwoQubitWeylDecomposition};
+
+const PI2: f64 = PI / 2.;


again, perhaps we can use here FRAC_PI_2 ?

ShellyGarion · 2026-03-23T08:50:36Z

+
+static IPZ: GateArray1Q = [[IM, C_ZERO], [C_ZERO, M_IM]];
+static IPY: GateArray1Q = [[C_ZERO, C_ONE], [C_M_ONE, C_ZERO]];
+static IPX: GateArray1Q = [[C_ZERO, IM], [IM, C_ZERO]];


again, perhaps these should be in common?

ShellyGarion · 2026-03-23T08:54:00Z

+// Python space.
+const TWO_QUBIT_SEQUENCE_DEFAULT_CAPACITY: usize = 21;
+
+static IPZ: GateArray1Q = [[IM, C_ZERO], [C_ZERO, M_IM]];


I think this also appears elsewhere in the code, so it's better to move it to common.

ShellyGarion · 2026-03-23T08:57:07Z

+    }
+}
+
+static K12R_ARR: GateArray1Q = [


perhaps this should be moved to common?

ShellyGarion · 2026-03-23T08:57:22Z

+    [c64(-FRAC_1_SQRT_2, 0.), c64(0., -FRAC_1_SQRT_2)],
+];
+
+static K12L_ARR: GateArray1Q = [


perhaps this should be moved to common?

ShellyGarion · 2026-03-23T08:59:01Z

+}
+
+#[pyfunction]
+pub fn local_equivalence(weyl: PyReadonlyArray1<f64>) -> PyResult<[f64; 3]> {


it's worth to add some docstring to this fucntion

This commit removes a duplicated method implementation on the TwoQubitBasisDecomposer. The struct had two methods, call_inner and generate_sequences, which were almost line for line identical. I believe this occured during a rebase when two concurrently developed PRs were touching the same file and the rebase was a bit off after one merged which resulted in the methods being duplicated. This was missed for quite some time and is what prompted Qiskit#15833 because the original two_qubit_decompose.rs file was so big it was very easy to overlook this duplication. This commit deletes the generate_sequence implementation and just uses the call_inner function.

mtreinish · 2026-03-23T12:01:11Z

@ShellyGarion none of your review comments apply to the contents of this PR. They all were on the parent of this PR #15833 (although I would have said on that review I wouldn't want to change the code while making the split) this PR is a very simple removal of the duplicated code. If you have suggested improvements for the two qubit decomposer code I'd suggest opening your own PR making the improvements.

ShellyGarion

I approve this change and will open another PR for code reorganization and small fixes.

alexanderivrii

LGTM! But not merging in case Shelly has additional comments.

alexanderivrii · 2026-03-23T12:26:25Z

-            .min_by(|(_idx1, fid1), (_idx2, fid2)| fid2.partial_cmp(fid1).unwrap())
-            .unwrap()
-            .0;
-        let best_nbasis = _num_basis_uses.unwrap_or(best_nbasis as u8);


As far as I can tell, there is no real difference between the two functions, while the surviving function is more efficient handling this line - does not eagerly compute best_nbasis.

Ah, Shelly has already approved the changes as well. :)

mtreinish requested a review from a team as a code owner March 19, 2026 01:33

mtreinish added Changelog: None Do not include in the GitHub Release changelog. synthesis Rust This PR or issue is related to Rust code in the repository labels Mar 19, 2026

ShellyGarion added this to the 2.5.0 milestone Mar 19, 2026

ShellyGarion added this to Qiskit 2.5 Mar 19, 2026

github-project-automation Bot moved this to Ready in Qiskit 2.5 Mar 19, 2026

ShellyGarion reviewed Mar 23, 2026

View reviewed changes

mtreinish force-pushed the remove-duplicated-sequence-generation branch from 73353ae to 84e88a7 Compare March 23, 2026 11:57

ShellyGarion approved these changes Mar 23, 2026

View reviewed changes

alexanderivrii approved these changes Mar 23, 2026

View reviewed changes

alexanderivrii added this pull request to the merge queue Mar 23, 2026

Merged via the queue into Qiskit:main with commit 572045f Mar 23, 2026
25 checks passed

github-project-automation Bot moved this from Ready to Done in Qiskit 2.5 Mar 23, 2026

ShellyGarion mentioned this pull request Mar 26, 2026

Organize two_qubit_decompose code #15880

Merged

Conversation

mtreinish commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented Mar 19, 2026

Uh oh!

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

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!

mtreinish commented Mar 23, 2026

Uh oh!

ShellyGarion left a comment

Choose a reason for hiding this comment

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mtreinish commented Mar 19, 2026 •

edited

Loading