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

Consective 1q param gates #8178

Closed

Conversation

rafal-pracht
Copy link
Contributor

Summary

Two or more Pauli rotation gates (Rx, Ry, Rz) that contain the parameters will be combined together.

Details and comments

@rafal-pracht rafal-pracht requested a review from a team as a code owner June 14, 2022 19:03
@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:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Jun 14, 2022

Pull Request Test Coverage Report for Build 2838137906

  • 20 of 26 (76.92%) changed or added relevant lines in 3 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.006%) to 84.039%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/parameterexpression.py 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
qiskit/pulse/library/waveform.py 3 89.36%
Totals Coverage Status
Change from base Build 2835258003: -0.006%
Covered Lines: 56328
Relevant Lines: 67026

💛 - Coveralls

@mtreinish mtreinish added this to the 0.22 milestone Jun 14, 2022
@Cryoris
Copy link
Contributor

Cryoris commented Jun 14, 2022

That's a great extension! Does this also collect cases where other single qubit gates are in between Pauli rotations, like RY(a) H RY(b)? And can we extend this to cover other parameterized single qubit gates like P (and ideally also U)?

# Delete the other nodes in the run
for current_node in run[idx:]:
dag.remove_op_node(current_node)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice idea. If I understand correctly, currently the code completely replaces chains of parameterized "rz", "ry", "rx" gates by "optimized" chains. Would it be possible to avoid this replacement if no (or only few) optimizations take place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This worked in that way previously. But in the code, the optimization is done by converting the gates to the matrix and multiplying the unitary of the gates. And at the end, the decomposition is done. So before my changes the parameters gates were not optimized.

@alexanderivrii
Copy link
Contributor

I am wondering if the CommutativeCancellation pass also tries to do something similar?

Copy link
Member

@ajavadia ajavadia 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. While this takes us a step further in optimizing parametric gates, it is still a bit limited. I opened an issue for a more general workflow: #8192

Another problem with this approach, as I commented below, is that it does not preserve basis gates.

Comment on lines +570 to +574
passmanager.append(Optimize1qGatesDecomposition(basis))
result = passmanager.run(qc)
expected = QuantumCircuit(1)
expected.rz(2 * phi, 0)
expected.ry(2 * phi, 0)
Copy link
Member

Choose a reason for hiding this comment

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

if the basis does not contain ry, then ry should not be emitted. This creates a bug as it does not conform to the spec of basis_gates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajavadia Do you suggest that it should do "merge" of gates only when there are in basis set?

Copy link
Member

Choose a reason for hiding this comment

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

Giving a basis means I want my output to be in that basis. So ry should be written as rz + sx here. If basis=None then I think it's fine to emit any gate, including ry itself.

@HuangJunye HuangJunye added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 21, 2022
@rafal-pracht rafal-pracht marked this pull request as draft June 22, 2022 17:38
@rafal-pracht rafal-pracht marked this pull request as ready for review June 28, 2022 16:49
@mtreinish mtreinish removed this from the 0.22 milestone Sep 27, 2022
@javabster
Copy link
Contributor

hi @rafal-pracht are you still working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

10 participants