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

Decomposition of single R gate in RR basis should be one R gate, not two #11304

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

sven-oxionics
Copy link
Contributor

@sven-oxionics sven-oxionics commented Nov 23, 2023

Summary

Currently, a single R gate may come out of RR-decomposition as two R gates, which is obviously not ideal.

This PR adds test cases for when this happens and contains the code changes to only emit one R gate when possible.

Details and comments

For example, RGate(0.1, 0.2) is currently decomposed (in RR basis) as

   ┌────────────────┐┌──────────┐
0: ┤ R(-3.0416,0.2) ├┤ R(π,0.2) ├
   └────────────────┘└──────────┘

Two R(𝜗, 𝜑) gates with the same 𝜑 parameter can be combined into one by simply adding up the 𝜗 values (giving us R(0.1, 0.2), unsurprisingly).

In terms of U(𝜗, 𝜑, 𝜆), it is the case when 𝜑 = -𝜆 that the two R-gates we construct have the same second parameter and therefore should be expressed as a single R gate. For example, U3Gate(0.1, 0.2, -0.2) currently produces this RR-decomposition:

   ┌───────────────────┐┌─────────────┐
0: ┤ R(-3.0416,1.7708) ├┤ R(π,1.7708) ├
   └───────────────────┘└─────────────┘

which also unnecessarily consists of two R gates instead of just one.

This commit adds the two examples above as unit tests, ensuring they RR-decompose two just one R gate, as well as the code changes to make these two new tests pass, along with all existing tests, of course.

The condition for this special case is that the 𝜑 parameters of the two R-gates we would emit are the same (thus mod_2pi(PI / 2. - lam)=mod_2pi(0.5 * (phi - lam + PI), simplified as mod_2pi((phi + lam) / 2)=0).

@sven-oxionics sven-oxionics requested review from ikkoham and a team as code owners November 23, 2023 09:03
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Nov 23, 2023
@CLAassistant
Copy link

CLAassistant commented Nov 23, 2023

CLA assistant check
All committers have signed the CLA.

@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:

Currently, a single R gate may come out of RR-decomposition as two R gates, which is obviously not ideal.

For example, `RGate(0.1, 0.2)` is currently decomposed (in RR basis) as

```
   ┌────────────────┐┌──────────┐
0: ┤ R(-3.0416,0.2) ├┤ R(π,0.2) ├
   └────────────────┘└──────────┘
```

Two `R(𝜗, 𝜑)` gates with the same 𝜑 parameter can be combined into one by simply adding up the 𝜗 values (giving us `R(0.1, 0.2)`, unsurprisingly).

In terms of `U(𝜗, 𝜑, 𝜆)`, it is the case when `𝜑 = -𝜆` that the two R-gates we construct have the same second parameter and therefore should be expressed as a single R gate.
For example, `U3Gate(0.1, 0.2, -0.2)` currently produces this RR-decomposition:

```
   ┌───────────────────┐┌─────────────┐
0: ┤ R(-3.0416,1.7708) ├┤ R(π,1.7708) ├
   └───────────────────┘└─────────────┘
```

which also unnecessarily consists of two R gates instead of just one.

This commit adds the two examples above as unit tests, ensuring they RR-decompose two just one R gate, as well as the code changes to make these two new tests pass, along with all existing tests, of course.

The condition for this special case is that the 𝜑 parameters of the two R-gates we would emit are the same (thus `mod_2pi(PI / 2. - lam)=mod_2pi(0.5 * (phi - lam + PI)`, simplified as `mod_2pi((phi + lam) / 2)=0`).
@coveralls
Copy link

coveralls commented Nov 23, 2023

Pull Request Test Coverage Report for Build 7030458574

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • 158 unchanged lines in 34 files lost coverage.
  • Overall coverage increased (+0.8%) to 87.242%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 1 92.17%
qiskit/circuit/library/generalized_gates/pauli.py 1 91.18%
qiskit/circuit/parametertable.py 1 87.4%
qiskit/providers/backend_compat.py 1 86.05%
qiskit/providers/backend.py 1 81.08%
qiskit/providers/basicaer/basicaertools.py 1 91.23%
qiskit/providers/basicaer/qasm_simulator.py 1 85.42%
qiskit/providers/job.py 1 56.9%
qiskit/quantum_info/states/statevector.py 1 89.69%
qiskit/synthesis/evolution/product_formula.py 1 97.56%
Totals Coverage Status
Change from base Build 6966472058: 0.8%
Covered Lines: 60785
Relevant Lines: 69674

💛 - 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.

Thanks for this! Sorry for the slightly delayed response. It's great to get these inefficiencies ironed out in the less-used decomposition targets.

Would you mind adding a feature release note mentioning the improved synthesis quality (reno new --edit rr-decomposition-synthesis or similar)? I can do it if it's easier, though.

@sven-oxionics
Copy link
Contributor Author

No problem, @jakelishman! I had a go. Feel free to edit if it doesn't quite sit right.

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.

This looks fine to me, thanks for finding it, and for contributing the fix!

@jakelishman jakelishman added this pull request to the merge queue Nov 29, 2023
@jakelishman jakelishman added performance Changelog: New Feature Include in the "Added" section of the changelog mod: quantum info Related to the Quantum Info module (States & Operators) labels Nov 29, 2023
@jakelishman jakelishman added this to the 1.0.0pre1 milestone Nov 29, 2023
@mtreinish mtreinish added the Rust This PR or issue is related to Rust code in the repository label Nov 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023
@jakelishman jakelishman added this pull request to the merge queue Nov 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 29, 2023
@jakelishman jakelishman added this pull request to the merge queue Nov 29, 2023
Merged via the queue into Qiskit:main with commit 3556740 Nov 29, 2023
14 checks passed
@sven-oxionics sven-oxionics deleted the rr-decomp-fix branch November 29, 2023 16:39
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 Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: quantum info Related to the Quantum Info module (States & Operators) performance Rust This PR or issue is related to Rust code in the repository
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants