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

Comments

@nelimee
Copy link

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

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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.

@nelimee

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

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.

@delapuente delapuente modified the milestone: 0.7 Jul 31, 2018
@ajavadia ajavadia self-assigned this Aug 9, 2018
@1ucian0 1ucian0 added this to To do in BasicAer via automation Oct 10, 2018
@jaygambetta jaygambetta assigned jaygambetta and unassigned ajavadia Dec 15, 2018
@jaygambetta jaygambetta moved this from To do to Assigned in BasicAer Dec 15, 2018
@chriseclectic

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Closed by #2755

BasicAer automation moved this from Assigned to Done Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
BasicAer
  
Done
7 participants
You can’t perform that action at this time.