-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make xmon gates parameterizable #427
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
Make xmon gates parameterizable #427
Conversation
Can't we just have Exp11Gate and ExpZGate subclass Rot11Gate and RotZGate? |
What is this? Why didn't this come up before? |
No. They define their matrices in different ways. (Different global phase.)
Not sure why, but mypy sometimes gets confused by these kinds of type values. This is worked around as part of #429 by moving the assignment to its own file. The really weird thing is it doesn't reproduce across machines. |
I don't understand. The matrices for Exp11Gate and ExpZGate are
and
respectively. How are these different from Rot11Gate and RotZGate? |
Travis is failing due to the same bug at #430 |
@@ -108,8 +107,12 @@ def transform_job(self, job): | |||
errors = np.random.random(self.realizations) < self.p | |||
if any(errors): | |||
key = self._parameter_name + str(error_number) | |||
new_error_gate = copy.deepcopy(gate) | |||
new_error_gate.half_turns = Symbol(key) | |||
if isinstance(gate, xmon_gates.ExpWGate): |
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.
We don't want special-case code for specific types of gates. A better approach is for the list of depolarizing gates to provide a mechanism for making each gate.
cirq/google/xmon_gates.py
Outdated
ops.PhaseableGate, | ||
PotentialImplementation): | ||
ops.Rot11Gate): |
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.
We want to avoid mixing the Xmon gate hierarchy with the the basic gate hierarchy, so that we are free to modify the definitions of the gates. For example, if we choose to change the global phase of the gate then this inheritance relationship breaks.
Yes, that does mean that the xmon gates are (nearly) copy-pastes of the usual gates.
cirq/google/xmon_gates.py
Outdated
def matrix(self): | ||
if not self.has_matrix(): | ||
raise ValueError("Don't have a known matrix.") | ||
return ops.RotZGate(half_turns=self.half_turns).matrix() |
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.
Huh. This line was actually wrong. It doesn't match the definition given in the docs.
cirq/google/xmon_gates.py
Outdated
def __hash__(self): | ||
return hash((ExpZGate, self.half_turns)) | ||
return hash((type(self), self.half_turns)) |
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 was the reasoning for changing this? This new definition means that a gate deriving from ExpZ is automatically not equal to any ExpZ gate, instead of just being e.g. an ExpZ with some extra methods.
Ah, the ExpZGate isn't supposed to have the same matrix. The Exp11Gate matrix being the same is something that I see as incidental as opposed to being related to how the Rot11Gate is defined. |
#430 has now fixed that mypy bug by pinning the version. Once that's merged and synced, your builds should start to pass. |
Ok I mostly reverted back to the old code and fixed the ExpZGate matrix. |
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.
LGTM
I'll switch these over to EigenGate at some point.
Fixes #428