Fix the CZ ops on the Clifford Tableau Implementation

[BichengYing]

Implementation idea of CZ is CZ(q0, q1) = H(q1)CNOT(q0, q1)H(q1) so the operations are just composition of these three.
The problem happens at rs of tableau. Original implementation seemed merge the rs of H and CNOT together. But it is not equivalent. See the fix code.

FYI, I discovered this because I keep encountering the failure in my Clifford decomposition test. I didn't realize it is the CZ implementation in the cirq has problem for a long time :(

[balopat]

LGTM, can you add a failing test case with the change here?

[BichengYing]

LGTM, can you add a failing test case with the change here?

What do you want to see the failing test case about? The case that new and old code returns different values?

[maffoo]

What do you want to see the failing test case about? The case that new and old code returns different values?

Add a test that would have failed with the old code, but that passes with this change. More generally, it would seem to me that if a bug like this can go undetected then we probably need to add more comprehensive tests, since there may be other issues beyond just the issue you identified.

[BichengYing]

I totally agree that we need more tests to catch potential issues. However, I don't think it is good to do it here.

My main reason is that Clifford tableau is kind of independent eco-system. The test case to add here will just be the loop of proof --- I think the tableau should look like this so I add a test to claim it. I believe this was the main reason that issues can be detected before.

In my opinion, a more proper and easier way to test is have a more "composite" test -- like using composite/decomposite/inverse, etc. of tableau (also the unitary matrix) to test over them, as I did in #4183.

As for this particular issue, you can check the following code

import cirq
import numpy as np
args1 = cirq.ActOnCliffordTableauArgs(
    tableau=cirq.CliffordTableau(num_qubits=2),
    axes=[],
    prng=np.random.RandomState(),
    log_of_measurement_results={},
)
args2 = cirq.ActOnCliffordTableauArgs(
    tableau=cirq.CliffordTableau(num_qubits=2),
    axes=[],
    prng=np.random.RandomState(),
    log_of_measurement_results={},
)
# Two args applied with same S
args1.axes=[1]
args2.axes=[1]
cirq.act_on(cirq.S, args=args1, allow_decompose=False)
cirq.act_on(cirq.S, args=args2, allow_decompose=False)

# Args1 uses H*CNOT*H 
args1.axes=[1]
cirq.act_on(cirq.H, args=args1, allow_decompose=False)
args1.axes=[0, 1]
cirq.act_on(cirq.CNOT, args=args1, allow_decompose=False)
args1.axes=[1]
cirq.act_on(cirq.H, args=args1, allow_decompose=False)
# Args2 uses CZ
args2.axes=[0, 1]
cirq.act_on(cirq.CZ, args=args2, allow_decompose=False)

# They suppose to be the same, but actually not
assert args1.tableau == args2.tableau, f"1: {args1.tableau._str_full_()} \n 2:{args2.tableau._str_full_()}"

[balopat]

I totally agree that we need more tests to catch potential issues. However, I don't think it is good to do it here.

My main reason is that Clifford tableau is kind of independent eco-system. The test case to add here will just be the loop of proof --- I think the tableau should look like this so I add a test to claim it. I believe this was the main reason that issues can be detected before.

In my opinion, a more proper and easier way to test is have a more "composite" test -- like using composite/decomposite/inverse, etc. of tableau (also the unitary matrix) to test over them, as I did in #4183.

As for this particular issue, you can check the following code

import cirq
import numpy as np
args1 = cirq.ActOnCliffordTableauArgs(
    tableau=cirq.CliffordTableau(num_qubits=2),
    axes=[],
    prng=np.random.RandomState(),
    log_of_measurement_results={},
)
args2 = cirq.ActOnCliffordTableauArgs(
    tableau=cirq.CliffordTableau(num_qubits=2),
    axes=[],
    prng=np.random.RandomState(),
    log_of_measurement_results={},
)
# Two args applied with same S
args1.axes=[1]
args2.axes=[1]
cirq.act_on(cirq.S, args=args1, allow_decompose=False)
cirq.act_on(cirq.S, args=args2, allow_decompose=False)

# Args1 uses H*CNOT*H 
args1.axes=[1]
cirq.act_on(cirq.H, args=args1, allow_decompose=False)
args1.axes=[0, 1]
cirq.act_on(cirq.CNOT, args=args1, allow_decompose=False)
args1.axes=[1]
cirq.act_on(cirq.H, args=args1, allow_decompose=False)
# Args2 uses CZ
args2.axes=[0, 1]
cirq.act_on(cirq.CZ, args=args2, allow_decompose=False)

# They suppose to be the same, but actually not
assert args1.tableau == args2.tableau, f"1: {args1.tableau._str_full_()} \n 2:{args2.tableau._str_full_()}"

I think this is a perfectly fine test case. Not sure what you mean by "be the loop of proof".

The key thing is that you found an issue, that is producing bad behavior. Even if the test case is not something what we would have come up during construction time (because lack of thinking about it, forgetfulness, complex, interconnected system, etc.) when we find a bug, we typically prove that something that should work isn't working via a test. Then we fix it and the test will stick around to protect against regressions.

[BichengYing]

My previous concern is the current unit test of act_on is just 1. create a tableau; 2. Apply some clifford gate on it. 3. assert the tableau after it is same as expected, which is just hard-code by hand.

The previous test is fine to some degrees. But the problem is that test is singleton, which doesn't align with other unit test in common_gate_test.py file. Second, that is just one case among 11520 cases of all two qubits clifford gates. (I can find this one test case by reversing the error of code.) My opinion is instead of adding this single unit test in here for one case of S gate alone, why not add more large-scale testings in Tableau to cover it?

[balopat]

My previous concern is the current unit test of act_on is just 1. create a tableau; 2. Apply some clifford gate on it. 3. assert the tableau after it is same as expected, which is just hard-code by hand.

Yes, the current test is not sufficient. The hard-coded examples are not bad per se, that is a quite typical approach to testing. Testing is not proof unfortunately. Proofs are in general hard to achieve programmatically, the best we can do is to cover some part of the parameter space, but in complex cases there is typically a chance that we missed something. Proactively identifying edge cases is important though.

The previous test is fine to some degrees. But the problem is that test is singleton, which doesn't align with other unit test in common_gate_test.py file.

The fact that no other test is like that in common_gate_test.py is not really a strong argument for not having a test. While sometimes we get the "typical tests" right and they inspire good coverage, in this case the design is obviously lacking.

Second, that is just one case among 11520 cases of all two qubits clifford gates. (I can find this one test case by reversing the error of code.) My opinion is instead of adding this single unit test in here for one case of S gate alone, why not add more large-scale testings in Tableau to cover it?

I am always open to better testing and higher coverage. And maybe it is worthwhile to enumerate and test all the 11520 two qubit tableaux.

My key issue is: we do not accept code changes that don't come with a test, otherwise anyone can change / remove that code without tests catching that behavior change. The single unit test (even though imperfect in coverage for the general case, but still better than what we have) or a larger test suite covering all 11520 cases both can cover this particular change. If you think #4183 will introduce all those tests, and it doesn't make sense to introduce this change without those tests, then this PR can be closed and I would include this change there.

[BichengYing]

My key issue is: we do not accept code changes that don't come with a test, otherwise anyone can change / remove that code without tests catching that behavior change. The single unit test (even though imperfect in coverage for the general case, but still better than what we have) or a larger test suite covering all 11520 cases both can cover this particular change. If you think #4183 will introduce all those tests, and it doesn't make sense to introduce this change without those tests, then this PR can be closed and I would include this change there.

I understand your point. Of course, I don't object to add more test. Here the discussion is to see how we can have more structural and systemic approach test to cover the Clifford ActOn behavior. Unit test is different from the math. symbol logics it is not possible to cover all cases, especially when we have larger and larger number of qubits. I do think randomization test in #4183 is one of the good way. We cannot cover all cases, but if the error of one gate implementation is wrong in x% of all tableau cases, k random tests may detect the error with 1 - (1-x%)^k probability.

Back for this PR, I added a the corresponding equivalency between cz and h_cx_h test. It actually looks good with new act_on API.