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

cu3 simulated matrix false on unitary simulator #546

Closed
nelimee opened this issue Jun 7, 2018 · 6 comments
Closed

cu3 simulated matrix false on unitary simulator #546

nelimee opened this issue Jun 7, 2018 · 6 comments
Assignees
Labels
priority: high type: enhancement It's working, but needs polishing

Comments

@nelimee
Copy link
Contributor

nelimee commented Jun 7, 2018

Informations

  • QISKit (Python SDK) version: 0.5.3
  • Operating system: Fedora 26
  • Python version: 3.6.5

What is the current behavior?

The cu3 gate simulated with the local_unitary_simulator does not produce the expected matrix for a cu3 gate.

Steps to reproduce the problem

Execute the script:

import qiskit
from qiskit import available_backends, get_backend,\
    execute, QuantumRegister, ClassicalRegister, QuantumCircuit
import numpy as np

def round_to_zero(vec, tol=2e-15):
    vec.real[abs(vec.real) < tol] = 0.0
    vec.imag[abs(vec.imag) < tol] = 0.0
    return vec

def controlled(U):
    n = U.shape[0]
    return np.vstack((np.hstack((np.identity(n), np.zeros((n,n)))),
                      np.hstack((np.zeros((n,n)), U))))

def U(theta, phi, lam):
    return np.array([
        [np.cos(theta/2),
         -np.exp(1.j*lam)*np.sin(theta/2)],
        [np.exp(1.j*phi)*np.sin(theta/2),
         np.exp(1.j*(phi+lam))*np.cos(theta/2)]
    ])

Q_SPECS = {
    "name": "TestCU3",
    "circuits": [
        {
            "name": "TestCU3",
            "quantum_registers": [
                {
                    "name": "ctrl",
                    "size": 1
                },
                {
                    "name": "qb",
                    "size": 1
                },
            ],
            "classical_registers": []
        }
    ],
}
Q_program = qiskit.QuantumProgram(specs=Q_SPECS)

circuit = Q_program.get_circuit("TestCU3")
qb = Q_program.get_quantum_register("qb")
ctrl = Q_program.get_quantum_register("ctrl")

theta, phi, lam = np.pi, np.pi/2, 3*np.pi/2

circuit.cu3(theta, phi, lam, ctrl[0], qb[0])

unitary_sim = get_backend('local_unitary_simulator')
res = execute([circuit], unitary_sim).result()
unitary = round_to_zero(res.get_unitary())
wanted_unitary = round_to_zero(controlled(U(theta, phi, lam)))

print("Unitary matrix from simulator:\n", unitary, sep='')
print("Unitary matrix from theory:\n", wanted_unitary, sep='')
print("Unitary matrices are the same:", np.allclose(unitary, wanted_unitary))

Output on my machine:

Unitary matrix from simulator:
[[1.+0.j 0.+0.j 0.+0.j 0.+0.j]
 [0.+0.j 0.+0.j 0.+0.j 0.-1.j]
 [0.+0.j 0.+0.j 1.+0.j 0.+0.j]
 [0.+0.j 0.-1.j 0.+0.j 0.+0.j]]
Unitary matrix from theory:
[[1.+0.j 0.+0.j 0.+0.j 0.+0.j]
 [0.+0.j 1.+0.j 0.+0.j 0.+0.j]
 [0.+0.j 0.+0.j 0.+0.j 0.+1.j]
 [0.+0.j 0.+0.j 0.+1.j 0.+0.j]]
Unitary matrices are the same: False

What is the expected behavior?

The two printed matrices should be equal.

Suggested solutions

The problem might be in:

  1. The unitary simulator.
  2. The definition of the cu3 gate in qiskit.extensions.standard.header.

I don't have any solution for the moment.

@chriseclectic
Copy link
Member

It appears there that the definition of cu3 in the standard header is actually implementing the a controlled-U where U = exp( -1j * (phi + lam)/2) * U3(theta, phi, lam). So there is a local phase which wouldn't matter for a single u3 (in this case it would be a global phase), but is important when implementing the controlled gate.

(Less important but you swapped the target and control qubits between the two cases, the "standard" cnot in qiskit is CX(0, 1) = [[1, 0, 0, 0], [0, 0, 0, 1], [0, 0, 1, 0], [0, 1, 0, 0]] since qubit 0 is to the right of the tensor product).

@jaygambetta
Copy link
Member

jaygambetta commented Jun 7, 2018

This is in the tutorial and i have not decided what we should do.

https://github.com/QISKit/qiskit-tutorial/blob/master/reference/tools/quantum_gates_and_linear_algebra.ipynb

i am leaning towards deleting this gate and making a new one that we call cU which is a four parameter gate that has the three phases in cU3 and a relative phase. I would also like to change the name of cU1(theta) to cphase(theta)

@rraymondhp
Copy link
Contributor

rraymondhp commented Jun 8, 2018

I am aware of the difference of u3 in the formula with what we have.

I wrote a note emphasizing the importance of global phase in control-U and its difference with just U. But that part is now missing. I will try to recover that part.

@diego-plan9 diego-plan9 added the type: enhancement It's working, but needs polishing label Jun 8, 2018
@nelimee
Copy link
Contributor Author

nelimee commented Jun 13, 2018

Okey so the c-U3 is false in the current implementation. I corrected the phase shift with a c-RZZ:

from sympy import pi
def crzz(circuit, theta, ctrl, target):
    circuit.cu1(theta, ctrl, target)
    circuit.cx(ctrl, target)
    circuit.cu1(theta, ctrl, target)
    circuit.cx(ctrl, target)

def crx(circuit, theta, ctrl, target):
    # Apply the supposed c-RX operation.
    circuit.cu3(theta, pi/2, 3*pi/2, ctrl, target)
    # For the moment, QISKit adds a phase to the U-gate, so we
    # need to correct this phase with a controlled Rzz.
    crzz(circuit, pi, ctrl, target)

About the representation of quantum gates, I think there should be a huge warning somewhere that the representation used by QISKit is not the one used in the literature.
Moreover, I think that a warning should be present in each method/function that expose the matrix representation of a gate. For the moment I found:

  1. The unitary simulator and the get_unitary method.
  2. The qiskit.mapper._compiling.two_qubit_kak method. For this one, the user does not know which representation is expected, whether the CX(0, 1) = [[1, 0, 0, 0], [0, 0, 0, 1], [0, 0, 1, 0], [0, 1, 0, 0]] one or the CX(0, 1) = [[1, 0, 0, 0], [1, 0, 0, 0], [0, 0, 0, 1], [0, 1, 0, 0]].

@jaygambetta
Copy link
Member

cU3 is ambiguous and i agree we need to make it clearer. A real cU should have 4 variables not 3. The tensor order has been described in many places (see the tutorials) and it will be clearer in the documentation when we redo it.

@chriseclectic
Copy link
Member

Closed by #2755

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high type: enhancement It's working, but needs polishing
Projects
None yet
Development

No branches or pull requests

7 participants