-
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
cirq.PauliStringPhasor
returns wrong unitary
#6612
Comments
I stumbled upon this when investigating a related request. Here is a proposed fix, with a unit test that checks the unitaries for all "dense" Pauli strings of len <= 4. As for why this extra Z... The algorithm used in
The bug was caused by wrongly involving the extra qubit in the CNOT. The proposed fix partitions qubits into a set affected by this process (those from the Pauli string) and another left aside. A sequence of I gates are applied to the other qubits, for the sole purpose of displaying a unitary of the correct size (otherwise the tensor products by Finally, in case the argument is (or simplifies to) I, a global phase shift is still applied. This might seem superfluous, but this ensures a correct behavior when this gate is controlled (taking advantage of the fact that |
oh I forgot to reply to this... it seems what you are looking for is |
I'm afraid this is still an issue!
EDIT: Should leave the positive eigenvalues untouched (default argument This is probably why @pgoiporia chose these arguments to make the issue obvious to see. Up to a global phase, the expected unitary for The code returns @pgoiporia, do you have other test cases you want to try? |
I agree that the unitary is wrong but the expected unitary for
so nothing happens there. for the negative eigen vectors we need to multiply them by -1 which essentially turns the The unitary of this gate is computed from its decomposition which seems to be incorrect. Cirq/cirq-core/cirq/ops/pauli_string_phasor.py Lines 354 to 371 in ab96766
|
I double checked. It is designed to map With The decomposition is correct and a textbook result, the only problem in the issue raised here is involving the extra qubits in it. Note that with For a matrix with identical eigenvalues like |
I added the |
No, the gate evaluates "exponential-like" expressions (exponentials when the positive and negative exponents are opposite). The code implements the circuit p. 210 of Nielsen&Chuang, with two differences:
From this circuit it is clear that involving a qubit in the CNOT is as if it had a burlemarxiste@09d37f4 does everything necessary to make it behave correctly with the identities. The unit test checks the unitaries against values computed by -- I hope you understand now why I focused my attention on this class: it is designed to be a gate to exponentiate Pauli strings, not to apply a Pauli String and then an extra phase, and from the Cirq documentation, it appears to be the "canonical" way of exponentiating a string, not PauliSumExponential: Since |
@burlemarxiste feel free to open a PR to fix the PauliStringPhasor issue though this won't count towards the fix for the other issue. The other issue needs to be fixed using DensePauliString this code block won't be valid after we change DensePauliString acted_upon_qubits = self.dense_pauli_string.on(*qubits).qubits
acted_upon_qubit_set = set(acted_upon_qubits)
identity_qubit_set = set(qubits) - acted_upon_qubit_set instead you may use an ancilla qubit (same as in the textbook). for that replace def _decompose_with_context_(
self, qubits: Sequence[cirq.Qid], context: Optional[cirq.DecompositionContext] = None
) -> cirq.OP_TREE:
if context is None:
context = cirq.DecompositionContext(cirq.ops.SimpleQubitManager())
(ancilla,) = context.qubit_manager.qalloc(1)
... # decomposition
context.qubit_manager.qfree([ancilla])
|
I don't see why the ancilia is needed.
acted_upon_qubits = self.dense_pauli_string.on(*qubits).qubits Agree, but in the current codebase it is how we get the set of "non-I" qubits from the In case of a serious overhaul of the I'm all for a |
One of these: A. The expected behavior of In that case:
B. The expected behavior of In that case:
C. The behavior of In that case:
This is particularly relevant if This is where the exponentiation happens: Cirq/cirq-core/cirq/ops/pauli_string.py Line 836 in 543d9cd
>>> cirq.unitary(np.exp(1j * cirq.X(q0) * cirq.X(q1) * np.pi / 4))
array([[0.70710678+0.j , 0. +0.j ,
0. +0.j , 0. +0.70710678j],
[0. +0.j , 0.70710678+0.j ,
0. +0.70710678j, 0. +0.j ],
[0. +0.j , 0. +0.70710678j,
0.70710678+0.j , 0. +0.j ],
[0. +0.70710678j, 0. +0.j ,
0. +0.j , 0.70710678+0.j ]])
>>> cirq.unitary(np.exp(1j * cirq.X(q0) * cirq.X(q0) * np.pi / 4))
array([[1.+0.j]])
>>> Getting the correct result for the second expression will involve more than changing the storage format of |
Description of the issue
I am trying to check the unitary of a
cirq.PauliStringPhasor
. I don't get what I expect.How to reproduce the issue
The unitary this generates represents a Z⊗Z, not Z⊗I
Cirq version
1.4.0.dev20240403011731
The text was updated successfully, but these errors were encountered: