Skip to content

Organize two_qubit_decompose code#15880

Merged
ShellyGarion merged 4 commits intoQiskit:mainfrom
ShellyGarion:update-two-qubit-decompose
Apr 1, 2026
Merged

Organize two_qubit_decompose code#15880
ShellyGarion merged 4 commits intoQiskit:mainfrom
ShellyGarion:update-two-qubit-decompose

Conversation

@ShellyGarion
Copy link
Copy Markdown
Member

@ShellyGarion ShellyGarion commented Mar 26, 2026

Summary

Following the discussions on #15833 and #15835, this PR continues the code organization of crates/synthesis/src/two_qubit_decompose

Details and comments

@ShellyGarion ShellyGarion added this to the 2.5.0 milestone Mar 26, 2026
@ShellyGarion ShellyGarion requested a review from a team as a code owner March 26, 2026 06:59
@ShellyGarion ShellyGarion 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 26, 2026
@qiskit-bot
Copy link
Copy Markdown
Collaborator

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

  • @Qiskit/terra-core
  • @levbishop

Copy link
Copy Markdown
Collaborator

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I agree to move re-used logic into a common.rs module, but if there's logic that's specific to a single file then I think it's clearer to keep it there. That shows where the logic is required, and importantly does not require making functions or consts pub. Having pub functions like this makes it harder to refactor and understand where it's actually used.

Comment thread crates/synthesis/src/two_qubit_decompose/basis_decomposer.rs Outdated
@ShellyGarion
Copy link
Copy Markdown
Member Author

In general I agree to move re-used logic into a common.rs module, but if there's logic that's specific to a single file then I think it's clearer to keep it there. That shows where the logic is required, and importantly does not require making functions or consts pub. Having pub functions like this makes it harder to refactor and understand where it's actually used.

My motivation here is that the main files for the decomposers (basis decomposer, controlled-u decomposer and weyl decomposer) should mainly include code that is specifically relevant for these decomposers.
This is the reason that I moved functions that seem to be more general (u4_to_su4, real_trace_transform) to common.rs, as well as some static matrices. I thought that putting these functions and matrices in common would be useful in case one adds more synthesis methods in the future (to prevent code duplications).

@Cryoris
Copy link
Copy Markdown
Collaborator

Cryoris commented Mar 26, 2026

I thought that putting these functions and matrices in common would be useful in case one adds more synthesis methods in the future (to prevent code duplications).

I agree we should move re-used code there, but it's not re-used yet -- do we have any other methods in the pipeline we want to add that would use this? There's obviously no hard rule on how to do this, but we also don't need to change things unnecessarily since we do have to review these changes in the end 🙂

@ShellyGarion
Copy link
Copy Markdown
Member Author

OK, so I moved back the consts and functions from common.rs to their original files, and added comments indicating these are helper functions / constant matrices.

Copy link
Copy Markdown
Collaborator

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the updates!

@ShellyGarion ShellyGarion added this pull request to the merge queue Apr 1, 2026
Merged via the queue into Qiskit:main with commit 3a1dc6e Apr 1, 2026
45 of 47 checks passed
@ShellyGarion ShellyGarion deleted the update-two-qubit-decompose branch April 1, 2026 08:49
@github-project-automation github-project-automation Bot moved this from Ready to Done in Qiskit 2.5 Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in the GitHub Release changelog. Rust This PR or issue is related to Rust code in the repository synthesis

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants