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

fix global phase in initializer #6236

Merged
merged 6 commits into from
Apr 21, 2021
Merged

fix global phase in initializer #6236

merged 6 commits into from
Apr 21, 2021

Conversation

ewinston
Copy link
Contributor

Summary

Fixes global phase in initializer.

fixes #5230

Details and comments

@ewinston ewinston requested a review from a team as a code owner April 15, 2021 06:03
Co-authored-by: Julien Gacon <gaconju@gmail.com>
@jlapeyre
Copy link
Contributor

  • Possibly clarifying comment: In the issue Initialize instruction does not preserve global phase #5230, I was worried about the global phase somehow making the circuit less efficient on hardware. But, it looks like, at least in some cases, the global_phase is carried along after applying decompose to a fixed point, in which case it shouldn't matter. That is, the gates are the same.
  • I notice that, for 1j * np.array([1.0, 0]), decompose replaces the RZ gate by a U1 gate with a different global phase. Does it make sense to avoid this decompose step by using a U1 gate from the outset?

@ewinston
Copy link
Contributor Author

@jlapeyre This was switched back to RZ as part of the migration away from u1, u2, u3 gates in qiskit as mentioned in issues #4106 #6165.

@jlapeyre
Copy link
Contributor

jlapeyre commented Apr 16, 2021

This was switched back to RZ

Ok. For the record: Before this PR the decomposition went U1 -> U.
With this PR, the decomposition goes RZ -> U1 -> U.
But, RZ will be changed to decompose directly to U, so that we'll have RZ -> U, in other words, no increased complexity.

LGTM

jlapeyre
jlapeyre previously approved these changes Apr 16, 2021
@Cryoris
Copy link
Contributor

Cryoris commented Apr 20, 2021

@ewinston Could you add a short bugfix releasenote on this? It would be nice to highlight that this now works with the correct global phase 🙂

@Cryoris
Copy link
Contributor

Cryoris commented Apr 20, 2021

Also: why is the global phase the sum of the remaining parameters? 🤔

@ewinston
Copy link
Contributor Author

@Cryoris The "remaining parameters" seemed to capture the phase factors which occur during each iteration of the decomposition.

Cryoris
Cryoris previously approved these changes Apr 21, 2021
Copy link
Contributor

@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! 🙂

@Cryoris Cryoris added automerge Changelog: Bugfix Include in the "Fixed" section of the changelog labels Apr 21, 2021
@mergify mergify bot merged commit b026000 into Qiskit:main Apr 21, 2021
@ewinston ewinston deleted the issue/5230 branch April 8, 2022 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog global-phase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initialize instruction does not preserve global phase
5 participants