Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add equivalences from CR<Pauli> to R<Paulis> #9507

Merged
merged 2 commits into from
Feb 1, 2023

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Feb 1, 2023

Summary

Add equivalences from controlled Pauli rotations to 2-Pauli rotations. This is particularly interesting e.g. for QPD applications where cuts are usually defined for 2-Pauli rotations or for compiling to the RZX gate and using a pulse-efficient decomposition. This could e.g. be used to compile a CRY gate to an efficient pulse sequence instead of 2 CX gates.

@Cryoris Cryoris added the Changelog: New Feature Include in the "Added" section of the changelog label Feb 1, 2023
@Cryoris Cryoris requested a review from a team as a code owner February 1, 2023 15:29
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@mtreinish
Copy link
Member

mtreinish commented Feb 1, 2023

It's not really related to this PR, as a standalone these equivalences are fine on their own. But one thing we are missing in the equivalence lib (it came up on slack) is targeting a CR<Pauli> basis. I think to fix it we can just define a rules from cx to crx, etc to fix that.

@coveralls
Copy link

coveralls commented Feb 1, 2023

Pull Request Test Coverage Report for Build 4066536314

  • 64 of 64 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.244%

Totals Coverage Status
Change from base Build 4065741968: 0%
Covered Lines: 67097
Relevant Lines: 78712

💛 - Coveralls

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

(For posterity: the correctness of the standard equivalence library is tested automatically, without needing to modify the suite.)

Comment on lines +287 to +298
# RXX to RZX
# ┌─────────┐ ┌───┐┌─────────┐┌───┐
# q_0: ┤0 ├ q_0: ┤ H ├┤0 ├┤ H ├
# │ Rxx(ϴ) │ ≡ └───┘│ Rzx(ϴ) │└───┘
# q_1: ┤1 ├ q_1: ─────┤1 ├─────
# └─────────┘ └─────────┘
theta = Parameter("theta")
rxx_to_rzx = QuantumCircuit(2)
rxx_to_rzx.h(0)
rxx_to_rzx.rzx(theta, 0, 1)
rxx_to_rzx.h(0)
_sel.add_equivalence(RXXGate(theta), rxx_to_rzx)
Copy link
Member

Choose a reason for hiding this comment

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

If this is correct then I don't like our naming convention for RZXGate - to me, that reads as a ZX bitstring interaction, i.e. the Z is on qubit 1. But whatever, even if it mattered what I thought, the ship's long since sailed.

Copy link
Contributor Author

@Cryoris Cryoris Feb 1, 2023

Choose a reason for hiding this comment

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

Tbh I never know which order it is and just end up trying until it fits 😅 but I agree that RZX should implement e^{i theta * ZX} and in our tensor order that would be Z on q_1..

Copy link
Member

@jakelishman jakelishman Feb 1, 2023

Choose a reason for hiding this comment

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

I think the argument is that it implements what our documentation calls $Z \otimes X$, where I think there's some conflation of the general tensor product $\otimes$ with the Kronecker product ordering we use to make concrete matrix representations.

But anyway, the ship's sailed, and it's unrelated to this PR!

Copy link
Member

Choose a reason for hiding this comment

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

Ah - Wikipedia explains:

the Kronecker product, sometimes denoted by $\otimes$

as opposed to:

the tensor product $V\otimes W$

I've never used \otimes to refer to the Kronecker product, and wasn't aware that was something people did - to me, it's always meant the arbitrary tensor product, which isn't tied to any particular representation of linear algebra.

@Cryoris
Copy link
Contributor Author

Cryoris commented Feb 1, 2023

@mtreinish I opened an issue to keep track of these missing equivalences: #9511

@mergify mergify bot merged commit 09aa5c1 into Qiskit:main Feb 1, 2023
pranay1990 pushed a commit to pranay1990/qiskit-terra that referenced this pull request Feb 9, 2023
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants