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

Implement GlobalPhaseGate #4697

Merged
merged 23 commits into from
Dec 13, 2021
Merged

Implement GlobalPhaseGate #4697

merged 23 commits into from
Dec 13, 2021

Conversation

daxfohl
Copy link
Contributor

@daxfohl daxfohl commented Nov 22, 2021

Implements GlobalPhaseOperation in terms of a GateOperation on a new class GlobalPhaseGate.

Mostly involved moving existing functions from the operation to the gate, and then having the operation call those methods under the hood.

xref #4683

@tanujkhattar

@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Nov 22, 2021
@CirqBot CirqBot added the size: M 50< lines changed <250 label Nov 22, 2021
@tanujkhattar tanujkhattar self-assigned this Nov 22, 2021
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

  1. Should we deprecate GlobalPhaseOperation in favour of GlobalPhaseGate? I don't think we need to keep both.

  2. I'd like to see what other maintainers think about the paradigm shift here -- i.e. we are introducing the first gate in Cirq which should be applied to an empty set of qubits (i.e. op = GlobalPhaseGate(1j).on()).

cc @dstrain115 @MichaelBroughton Thoughts?

cirq-core/cirq/linalg/operator_spaces.py Outdated Show resolved Hide resolved
cirq-core/cirq/ops/global_phase_op.py Show resolved Hide resolved
@tanujkhattar
Copy link
Collaborator

It came up during discussion on Cirq sync that GlobalPhaseGate should act on all qubits instead of acting on no qubits. This is important because now we can have a Controlled CircuitOperation; where the underlying circuit has a global phase. Since we wrap the circuit in a CircuitOperation and apply controls on it, the phase is not global anymore (and hence can influence the output of the circuit). A GlobalPhaseGate acting on all qubits of the circuit can capture this global -> local conversion.

@daxfohl
Copy link
Contributor Author

daxfohl commented Dec 1, 2021

That seems like a different kind of gate though, like a TargetedPhaseShiftGate, right? GlobalPhaseGate with no qubits is designed to be a drop-in replacement for GlobalPhaseOp. If we start saying GlobalPhaseGate has different behaviors than GlobalPhaseOp depending on what qubits you provide, then the migration path is much rougher (or impossible in cases where you don't know your qubits in advance).

@tanujkhattar
Copy link
Collaborator

That seems like a different kind of gate though

That's right. The comment was that we should probably work directly towards deprecating GlobalPhaseOp in favour of the new gate which acts on all qubits instead of first deprecating GlobalPhaseOp in favour of the drop-in replacement (i.e. GlobalPhaseGate as implemented now) and then deprecating it again in favour of the TargetedPhaseShiftGate (or whatever we might end up calling the new gate).

@daxfohl
Copy link
Contributor Author

daxfohl commented Dec 2, 2021

Would we need to deprecate one over the other though? Would we not keep both? Or was the idea of a GlobalPhaseOp just unsound from the start?

If we do need to replace it with something else, what does that new thing do with the qubits it is given? I'm not seeing how those qubits affect the unitary or are otherwise used.

@tanujkhattar
Copy link
Collaborator

tanujkhattar commented Dec 2, 2021

The original concern I had in mind was that things might not behave as expected if we have a CircuitOperation with a global phase and we control it with other qubits, thus turning the global phase into a relative phase which can impact the output of the final circuit.

However, on digging a bit more, it seems like this particular use case works as expected right now because controlled CircuitOperations decompose into controlled versions of the underlying operations in the circuit and a controlled global phase op behaves like a z rotation, which is expected.

We should still add a custom controlled_by override to GlobalPhaseOperation and return a ZPowGate to make it more clear (similar to how Z.controlled_by returns a CZ).

cc @95-martin-orion Since you originally brought this up in the cirq sync -- do you see any other case where the combination of GlobalPhaseOperation + CircuitOperation isn't behaving as expected?

@95-martin-orion
Copy link
Collaborator

cc @95-martin-orion Since you originally brought this up in the cirq sync -- do you see any other case where the combination of GlobalPhaseOperation + CircuitOperation isn't behaving as expected?

You hit the main case I was concerned about, that being the different meanings of "global" when nesting circuits.

@daxfohl
Copy link
Contributor Author

daxfohl commented Dec 2, 2021

So what I'm hearing is that since the existing behavior of GlobalPhaseOp is "local" as desired when placed in a controlled subcircuit, we can give GlobalPhaseGate the exact same implementation, meaning there is no need to have it accept qubits in the constructor because they would have no effect and would just be confusing. Is that a correct interpretation?

Should the name be changed to "PhaseShiftGate", so as not to imply a truly global behavior?

@tanujkhattar
Copy link
Collaborator

as desired when placed in a controlled subcircuit, we can give GlobalPhaseGate the exact same implementation

Yes, that's right. I'd prefer to stick with GlobalPhaseGate since it's much more descriptive in the common case of adding a global phase to the circuit which shouldn't impact the output. (controlled sub circuits is a special case, and in this case the GlobalPhaseGate would automatically change to a ZPow/controlled gate).

@daxfohl
Copy link
Contributor Author

daxfohl commented Dec 3, 2021

That made this PR go from medium to 32 files :) but most of the changes were mechanical. Change GlobalPhaseOp() -> global_phase_op(), and isinstance(x.untagged, GlobalPhaseOp) -> isinstance(x.gate, GlobalPhaseGate).

@daxfohl daxfohl requested a review from tanujkhattar December 7, 2021 20:28
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Looks good overall. Left a round of comments, mainly around using GlobalPhaseGate in Gateset.

cirq-core/cirq/ops/gateset.py Show resolved Hide resolved
cirq-core/cirq/ops/gateset.py Show resolved Hide resolved
cirq-core/cirq/ops/global_phase_op.py Outdated Show resolved Hide resolved
cirq-core/cirq/ops/global_phase_op.py Outdated Show resolved Hide resolved
cirq-core/cirq/ops/global_phase_op.py Show resolved Hide resolved
cirq-core/cirq/ops/global_phase_op.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

LGTM

cirq-google/cirq_google/calibration/workflow.py Outdated Show resolved Hide resolved
cirq-core/cirq/ops/gateset.py Show resolved Hide resolved
@tanujkhattar tanujkhattar merged commit e207415 into quantumlib:master Dec 13, 2021
@daxfohl daxfohl deleted the gates-gp branch December 13, 2021 20:49
MichaelBroughton pushed a commit to MichaelBroughton/Cirq that referenced this pull request Jan 22, 2022
Implements GlobalPhaseOperation in terms of a GateOperation on a new class GlobalPhaseGate.

Mostly involved moving existing functions from the operation to the gate, and then having the operation call those methods under the hood.
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Implements GlobalPhaseOperation in terms of a GateOperation on a new class GlobalPhaseGate.

Mostly involved moving existing functions from the operation to the gate, and then having the operation call those methods under the hood.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Implements GlobalPhaseOperation in terms of a GateOperation on a new class GlobalPhaseGate.

Mostly involved moving existing functions from the operation to the gate, and then having the operation call those methods under the hood.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining. size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants