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

Assume PhasedXPowGate is different from XPowGate and YPowGate #7070

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

codrut3
Copy link
Contributor

@codrut3 codrut3 commented Feb 15, 2025

A similar change was done for PhasedISwapPowGate. This addresses issue #6528.

…ue_equality protocol.

A similar change was done for PhasedISwapPowGate. This addresses issue quantumlib#6528.
@codrut3 codrut3 requested review from vtomole and a team as code owners February 15, 2025 20:23
@codrut3 codrut3 requested a review from viathor February 15, 2025 20:23
@CirqBot CirqBot added the size: S 10< lines changed <50 label Feb 15, 2025
@@ -243,21 +243,12 @@ def _canonical_exponent(self):
return self._exponent % period

def _value_equality_values_cls_(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be removed completely? I assume by the description in #4585, removing it entirely would have the same effect. But I could be wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 - the _value_equality_values_cls_ can be indeed removed provided manual_cls in the decorator is changed to the False default. The _value_equality_approximate_values_ defaults to _value_equality_values_ so it can be removed as well.

diff --git a/cirq-core/cirq/ops/phased_x_gate.py b/cirq-core/cirq/ops/phased_x_gate.py
index d769b863..51bc5f39 100644
--- a/cirq-core/cirq/ops/phased_x_gate.py
+++ b/cirq-core/cirq/ops/phased_x_gate.py
@@ -30,3 +30,3 @@ from cirq.ops import raw_types
 
-@value.value_equality(manual_cls=True, approximate=True)
+@value.value_equality(approximate=True)
 class PhasedXPowGate(raw_types.Gate):
@@ -244,5 +244,2 @@ class PhasedXPowGate(raw_types.Gate):
 
-    def _value_equality_values_cls_(self):
-        return PhasedXPowGate
-
     def _value_equality_values_(self):
@@ -250,5 +247,2 @@ class PhasedXPowGate(raw_types.Gate):
 
-    def _value_equality_approximate_values_(self):
-        return self.phase_exponent, self._canonical_exponent, self._global_shift
-
     def _json_dict_(self) -> Dict[str, Any]:

@codrut3 codrut3 requested review from wcourtney and verult as code owners March 3, 2025 22:24
@codrut3
Copy link
Contributor Author

codrut3 commented Mar 3, 2025

Hi Dax and Pavol,

Thank you a lot for your feedback! I updated the code following your suggestion.

However, in the meantime I noticed that several tests were failing. I updated the tests so that you can see the effect, but I think this change is overall problematic.
The problem is that:

  • circuit optimization,
  • merge gate operations,
  • and conversion to proto,

all transform gates X and Y to PhasedXPowGate. This worked well until now because the gate was correctly interpreted as X or Y, for example when printing the circuit. However, with this change this is no longer the case. In particular, users will be surprised to optimize a circuit and instead of X, end up with a PhasedXPowGate(phase_exponent=0). It's harder to read, and it's a poor user experience.

I am inclined to drop this pull request. Let me know what you think and if there is a way around this problem.

@daxfohl
Copy link
Collaborator

daxfohl commented Mar 4, 2025

Should an optimizer have a final stage where it puts all the gates in canonical form perhaps?

@pavoljuhas
Copy link
Collaborator

Should an optimizer have a final stage where it puts all the gates in canonical form perhaps?

Indeed, that would be nice. This change actually shows the affected transformations could work better.

@tanujkhattar - would you have some tips for a transformer that does such conversion,
e.g., cirq.PhasedXPowGate(phase_exponent=0) to cirq.X?

Here is an example code adapted from a unit test -

import cirq
q = cirq.q(0)
c0 = cirq.Circuit([cirq.Moment([cirq.Z(q) ** 0.5]), cirq.Moment([cirq.Y(q) ** 0.25])])
c1 = cirq.eject_z(c0)
print(repr(c1))

output:

cirq.Circuit([
    cirq.Moment(
        cirq.PhasedXPowGate(phase_exponent=0.0, exponent=0.25).on(cirq.LineQubit(0)),
    ),
    cirq.Moment(
        cirq.S(cirq.LineQubit(0)),
    ),
])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants