-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add cirq.PhasedXPowZPowGate #2566
Conversation
- Add cirq.CircuitDiagramInfoArgs.format_real helper method This gives a canonical single qubit operation to optimize into when doing microwave rotations with virtual Zs
cirq/ops/phased_x_z_gate.py
Outdated
x_exponent: The $x$ in $Z^z Z^a X^x Z^{-a}$. | ||
z_exponent: The $z$ in $Z^z Z^a X^x Z^{-a}$. Note that the $Z^z$ | ||
operation happens last. | ||
axis_phase_exponent: The $a$ $Z^z Z^a X^x Z^{-a}$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like these argument names
…ecompose - This code is not very performant, but it does the job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. Just a few minor comments.
|
||
@value.value_equality(approximate=True) | ||
class PhasedXZPowGate(gate_features.SingleQubitGate): | ||
"""A single qubit operation expressed as $Z^z Z^a X^x Z^{-a}$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This expression is matrix multiplication with time going to the left? Would be nice to make this clear so readers don't misinterpret this as a "circuit" with time going to the right. (This is implicit in the constructor docstring for z_exponent
, but would be good to be clear up front.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarified.
cirq/ops/phased_x_z_gate.py
Outdated
axis_phase_exponent: Union[numbers.Real, sympy.Basic]) -> None: | ||
""" | ||
Args: | ||
x_exponent: The $x$ in $Z^z Z^a X^x Z^{-a}$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd suggest not repeating this expression. Can say something like The $x$ in the gate expression above
and refer back to the class docstring. Same for the following params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In many contexts, "above" will not refer to anything. For example, autocomplete popups in IDEs listing the parameter will not also list the docstring of the class. I tried to make these clearer.
z = self.z_exponent | ||
a = self.axis_phase_exponent | ||
|
||
# Canonicalize X exponent into (-1, +1]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also canonicalize x to [0, 1]
and adjust the axis phase accordingly if needed. This is generally how we do things internally so that the pulse amplitude is always a positive number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. The exact canonicalization is not too important; it's only really relevant for equality. The user doesn't see it.
if z > 1: | ||
z -= 2 | ||
|
||
# Canonicalize axis phase exponent into (-0.5, +0.5]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, it would match our internal convention more closely if we instead canonicalize x to [0, 1]
and a to (-1, 1]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. The exact canonicalization is not too important; it's only really relevant for equality. The user doesn't see it.
cirq/ops/phased_x_z_gate.py
Outdated
q = qubits[0] | ||
yield ops.Z(q)**-self._axis_phase_exponent | ||
yield ops.X(q)**self._x_exponent | ||
yield ops.Z(q)**(self._z_exponent + self._axis_phase_exponent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: write this as self._axis_phase_exponent + self._z_exponent
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
cirq/ops/phased_x_z_gate.py
Outdated
|
||
|
||
@value.value_equality(approximate=True) | ||
class PhasedXZPowGate(gate_features.SingleQubitGate): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should change the name here; I think of a "Pow" gate as a gate that can be raised to an exponent, which is not the case here, at least as implemented. I guess we have a similar issue with PhaseXPowGate
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed to PhasedXPowZPowGate
.
Alternatives could be...
cirq.google.MicrowaveGate
cirq.google.MicrowaveFrameGate
PhysicalXVirtualZsGate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like WZ
, but I guess we've moved away from W
for the phase X gate, so that's probably too cryptic...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PhasedXPowZPowGate
sounds like something someone would come up while satirizing cirq's gate naming conventions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about PhasedXZGate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to PhasedXZGate
.
But what about AbstractPhasedXZOperationFactory
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminds me of https://hackernoon.com/a-factoryfactoryfactory-in-production-822478b5afbd by a former coworker :)
cirq/ops/phased_x_z_gate.py
Outdated
args: 'cirq.CircuitDiagramInfoArgs') -> str: | ||
"""See `cirq.SupportsCircuitDiagramInfo`.""" | ||
return (f'PhXZ(' | ||
f'p={args.format_real(self._axis_phase_exponent)},' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call this a
instead to match constructor args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
# Conflicts: # cirq/protocols/circuit_diagram_info_protocol.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good. I assume follow on work will tackle the updates to quantum engine gatesets?
This gives a canonical single qubit operation to optimize into when doing microwave rotations with virtual Zs