Split two qubit decomposer file into separate files#15833
Merged
alexanderivrii merged 2 commits intoQiskit:mainfrom Mar 22, 2026
Merged
Split two qubit decomposer file into separate files#15833alexanderivrii merged 2 commits intoQiskit:mainfrom
alexanderivrii merged 2 commits intoQiskit:mainfrom
Conversation
This commit takes the single crates/synthesis/src/two_qubit_decompose.rs file and splits it into a directory that contains several submodule files. The original file kind of organically mirrored the python structure which was to have all the two qubit decomposer implementations in a single giant file. This was an anti-pattern in python and has led to us missing many details over time because it was impossible to keep the entire contents of the file in your head or on screen. This has also translated to the Rust version of this module as I recently discovered during the development of Qiskit#13419 that there is a duplicated method with two different names on the two qubit basis decomposer. The implementations actually are right next to each other in the impl block. Nobody noticed because the file was too big. This commit doesn't fix this as I wanted this migration to not change any code if possible. The new structure of this module is to break out each decomposer implementation into a standalone file. If we add new decomposers in the future they will naturally fit into a new file. The only small changes made to the code in this commit are mostly about visibility of functions and structs because things need to be at least pub(super) if they cross a file boundary now. A few places were made fully pub or pub(crate) depending on how they're used. Python things were made full pub to indicate that even if pyo3 doesn't require it. There was one small code change made as part of this in the `TwoQubitBasisDecomposer::__getnewargs__` method it was previously accessing the `TwoQubitWeylDecomposer`'s `unitary_matrix` field and then converting that to a python array and finally a Py<PyAny>. However there was a dedicated method for that already `TwoQubitWeylDecomposer::unitary_matrix()`, I switched the implementation to just use that method instead. I originally did this in order to avoid changing the visibility on the unitary_matrix field, but I had to do that for other places, but I decided not to change this back because it's a more natural fit.
mtreinish
added
the
Changelog: None
mtreinish
added
the
Rust
Collaborator
|
One or more of the following people are relevant to this code:
|
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Mar 19, 2026
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.
alexanderivrii
approved these changes
Mar 22, 2026
Member
alexanderivrii
left a comment
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Matthew - splitting this absolutely gigantic file into multiple smaller files makes a lot of sense. I could spot several functions that could probably be moved to a more common place, but it can be postponed till we need to use these functions from other files.
Comment on lines
+43
to
+46
| /// invert 1q gate sequence | ||
| fn invert_1q_gate( | ||
| gate: (StandardGate, SmallVec<[f64; 3]>), | ||
| ) -> (PackedOperation, SmallVec<[f64; 3]>) { |
Member
There was a problem hiding this comment.
Minor: this function should probably be in common.rs, but we can move it later when we need it from a different file.
Member
There was a problem hiding this comment.
I'm not sure since we don't use it in the basis decomposer.
| Ok((real_map, circ)) | ||
| } | ||
|
|
||
| fn compute_unitary(sequence: &TwoQubitSequenceVec, global_phase: f64) -> Array2<Complex64> { |
Member
There was a problem hiding this comment.
This function as well could be moved to common.rs, but we don't need to do this right away.
Member
There was a problem hiding this comment.
by the comments inside it, it seems too specific for x,sx,rz synthesis, so I left it where it is.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
|
||
| const PI2: f64 = PI / 2.; | ||
| const PI4: f64 = PI / 4.; | ||
| const TWO_PI: f64 = 2.0 * PI; |
Member
There was a problem hiding this comment.
why don't we use here FARC_PI_2, FARC_PI_4 etc.? this can be done in a follow-up.
| const PI4: f64 = PI / 4.; | ||
| const PI32: f64 = 3.0 * PI2; | ||
| const TWO_PI: f64 = 2.0 * PI; | ||
| const C1: c64 = c64 { re: 1.0, im: 0.0 }; |
Member
There was a problem hiding this comment.
same comment about the constants, but can be also done in a follow-up
ShellyGarion
approved these changes
Mar 22, 2026
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Mar 23, 2026
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.
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Mar 23, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This commit takes the single crates/synthesis/src/two_qubit_decompose.rs file and splits it into a directory that contains several submodule files. The original file kind of organically mirrored the python structure which was to have all the two qubit decomposer implementations in a single giant file. This was an anti-pattern in python and has led to us missing many details over time because it was impossible to keep the entire contents of the file in your head or on screen. This has also translated to the Rust version of this module as I recently discovered during the development of #13419 that there is a duplicated method with two different names on the two qubit basis decomposer. The implementations actually are right next to each other in the impl block. Nobody noticed because the file was too big. This commit doesn't fix this as I wanted this migration to not change any code if possible. The new structure of this module is to break out each decomposer implementation into a standalone file. If we add new decomposers in the future they will naturally fit into a new file.
The only small changes made to the code in this commit are mostly about visibility of functions and structs because things need to be at least pub(super) if they cross a file boundary now. A few places were made fully pub or pub(crate) depending on how they're used. Python things were made full pub to indicate that even if pyo3 doesn't require it. There was one small code change made as part of this in the
TwoQubitBasisDecomposer::__getnewargs__method it was previously accessing theTwoQubitWeylDecomposer'sunitary_matrixfield and then converting that to a python array and finally a Py. However there was a dedicated method for that alreadyTwoQubitWeylDecomposer::unitary_matrix(), I switched the implementation to just use that method instead. I originally did this in order to avoid changing the visibility on the unitary_matrix field, but I had to do that for other places, but I decided not to change this back because it's a more natural fit.Details and comments