quantumlib / Cirq

A Python framework for creating, editing, and invoking Noisy Intermediate Scale Quantum (NISQ) circuits.
Apache License 2.0
4.24k stars 1.01k forks source link

Fix "Xa*Zb*Zb*Zb works but Xa*Zb**3" #6618

Closed rodolfocarobene closed 3 months ago

rodolfocarobene commented 4 months ago

Hi, I'm trying to close #5715 as part of the UnitaryHack2024.

First, I was considering only positive integers, so that with the power operator was possible to replicate exactly what is possible to do with the multiplication operator (as in the initial issue). Then I saw the solution proposed over a year ago and in my opinion that should be considered good enough: while it is true that the even powers present a "weird" behavior (they lose the information on the qubit, since they are the identity) this is the same with the multiplication and should be expected. I would argue that having different outputs would be more of a problem

import cirq
from cirq import ParamResolver
a, b = cirq.LineQubit.range(2)
Xa, Zb = cirq.X(a), cirq.Z(b)

print(Xa * Zb ** 3)
print(Xa * Zb * Zb * Zb)
print(Xa)
print(Xa * Zb ** 0)
print(Zb ** 2)
print(Zb * Zb)
X(q(0))*Z(q(1))
X(q(0))*Z(q(1))
X(q(0))
X(q(0))
I
I
google-cla[bot] commented 4 months ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

rodolfocarobene commented 4 months ago

Thanks @NoureldinYosri ! I've now added the test mentioned and checked locally that all the CI pass with . ./check/all --apply-format-changes

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.80%. Comparing base (16de069) to head (96d268f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6618 +/- ## ========================================== - Coverage 97.81% 97.80% -0.01% ========================================== Files 1067 1067 Lines 91518 91550 +32 ========================================== + Hits 89517 89545 +28 - Misses 2001 2005 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rodolfocarobene commented 4 months ago

that is update mul of each of them to know what to do

Hi @NoureldinYosri , I agree that would solve the specific problem of the issue, but that would still remain the "equality problem" that is instead fixed now:

import cirq
q = cirq.q(0)
op = cirq.X(q)

op * op == op ** 2  # this would be false while it is actually true

That would be just because pauli gates and the pow gates didn't actually got multiplied

NoureldinYosri commented 4 months ago

@rodolfocarobene the original issue was about multiplication ... I'm okay with keeping == out for now ... we can test equality in an indirect way by checking their unitaries ... lets do the changes to only __mul__ in the affected classes

the reason for this is that the different return types for __pow__ is a bad practice and we shouldn't do it if we can

rodolfocarobene commented 4 months ago

Hi @NoureldinYosri, what do you think of the current solution? Now only the case where multiplication and exponential can be equivalent (exponent is integer) are handled, while other cases are treated as before.

This should solve the problem of the issue, let me know if I can improve anything :-)

rodolfocarobene commented 3 months ago

Thanks for the suggestions, I should have implemented them