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

Global phase uc #6226

Closed
wants to merge 16 commits into from
Closed

Global phase uc #6226

wants to merge 16 commits into from

Conversation

adjs
Copy link
Contributor

@adjs adjs commented Apr 14, 2021

Summary

QuantumCircuit.uc does not preserve global phase. This PR adds the gate $P = e^{-i\pi/4}$ (see fig. 3 of arXiv:quant-ph/0410066).

Details and comments

@adjs adjs requested a review from a team as a code owner April 14, 2021 13:53
@Cryoris
Copy link
Contributor

Cryoris commented Apr 14, 2021

If it doesn't break anything I think we should just always add the correct global phase. It looks more like a bug to me and not a feature we want to preserve 🙂

@adjs
Copy link
Contributor Author

adjs commented Apr 14, 2021

Hi @Cryoris , it breaks test_isometry_inverse in test/python/circuit/test_isometry.py
I didn't check the reason.

@Cryoris
Copy link
Contributor

Cryoris commented Apr 14, 2021

That's odd, nothing should work based off the global phase not being conserved. I think we should try get to the bottom of this and then hopefully remove the argument.

@adjs
Copy link
Contributor Author

adjs commented Apr 15, 2021

I converted the isometry into a gate (instead of Instruction). With this modification the isometry.inverse works as expected.

The change from instruction to gate will not break anyone's code because a gate is also an instruction and isometry is a unitary operation. Let me know if this is the best strategy.


def _define(self):
# TODO The inverse().inverse() is because there is code to uncompute (_gates_to_uncompute)
# an isometry, but not for generating its decomposition. It would be cheaper to do the
# later here instead.
gate = self.inverse().inverse()
gate = self.iso_inverse()
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be something like inv_gate for clarity?

@adjs adjs requested a review from ewinston April 19, 2021 10:49
@ewinston
Copy link
Contributor

I converted the isometry into a gate (instead of Instruction). With this modification the isometry.inverse works as expected.

The change from instruction to gate will not break anyone's code because a gate is also an instruction and isometry is a unitary operation. Let me know if this is the best strategy.

Since generally speaking an isometry is not always unitary could you keep this as an instruction?

@adjs
Copy link
Contributor Author

adjs commented Apr 26, 2021

Thanks for the comment @ewinston. I reverted isometry to an instruction.

I thought that the isometry should be a gate, because its implementation contains only unitary parts.

@ewinston
Copy link
Contributor

ewinston commented May 4, 2021

Thanks for the update @adjs.

ewinston
ewinston previously approved these changes May 4, 2021
This reverts commit bc6956d, reversing
changes made to be42d08.
@adjs
Copy link
Contributor Author

adjs commented Oct 11, 2021

I made a commit that messed up this PR. Then I'll open another one in place of this one.

@HuangJunye HuangJunye added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 21, 2022
@adjs adjs mentioned this pull request Jun 24, 2022
@adjs
Copy link
Contributor Author

adjs commented Jun 24, 2022

I'm closing this PR in favour of PR #8231

@adjs adjs closed this Jun 24, 2022
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 global-phase
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants