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

Error in extracting qasm code from a controlled UnitaryGate #4623

Closed
MatthewGregoire42 opened this issue Jun 26, 2020 · 10 comments · Fixed by #9953
Closed

Error in extracting qasm code from a controlled UnitaryGate #4623

MatthewGregoire42 opened this issue Jun 26, 2020 · 10 comments · Fixed by #9953
Labels
bug Something isn't working mod: qasm2 Relating to OpenQASM 2 import or export

Comments

@MatthewGregoire42
Copy link

MatthewGregoire42 commented Jun 26, 2020

Information

  • Qiskit Terra version: 0.14.2
  • Python version: 3.6.8
  • Operating system: Microsoft Windows [Version 10.0.18363.900]

What is the current behavior?

When I run the following code, I get the corresponding error:

from qiskit.extensions import UnitaryGate
import numpy as np

U = np.array([[1, 0, 0, 0],
              [0, 1, 0, 0],
              [0, 0, 0, 1],
              [0, 0, 1, 0]])

gate = UnitaryGate(U).control()

print(gate.qasm())
Traceback (most recent call last):
  File "c:/Users/matth/git/QiskitSummerJam/Qiskit/error.py", line 11, in <module>
    print(gate.qasm())
  File "C:\Users\matth\AppData\Local\Programs\Python\Python36\lib\site-packages\qiskit\circuit\instruction.py", line 314, in qasm
    [pi_check(i, ndigits=8, output='qasm') for i in self.params]))
  File "C:\Users\matth\AppData\Local\Programs\Python\Python36\lib\site-packages\qiskit\circuit\instruction.py", line 314, in <listcomp>
    [pi_check(i, ndigits=8, output='qasm') for i in self.params]))
  File "C:\Users\matth\AppData\Local\Programs\Python\Python36\lib\site-packages\qiskit\circuit\tools\pi_check.py", line 134, in pi_check
    complex_inpt = complex(inpt)
TypeError: only length-1 arrays can be converted to Python scalars

It looks like gate.params returns the unitary matrix U, and then pi_check tries to apply complex(U), which results in the error.

Steps to reproduce the problem

Run the code above.

What is the expected behavior?

No error should occur.

Suggested solutions

This looks like the same error as reported in #4447. It looks like the problem arises because gate.params is simply the numpy unitary matrix, but the .qasm() method assumes that the parameters are all floats and calls pi_check on the unitary matrix. I'm not sure if this is an issue with the .qasm() method, or with the way parameters are stored for the controlled gate object. (When you remove the .control() call from the definition of gate, the code runs as expected.)

@MatthewGregoire42 MatthewGregoire42 added the bug Something isn't working label Jun 26, 2020
@faisaldebouni
Copy link
Contributor

faisaldebouni commented Jun 30, 2020

UnitaryGate.control() returns an instance of ControlledGate (not UnitaryGate), which doesn't override a qasm() function. thus qasm() reverts back to instruction.qasm() which assumes that self.param is an array of qarg. while UnitaryGate.control() stores the matrix representation in params

I think the solution is either to:

  • option 1: make ControlledGate extends UnitaryGate so it inherits the qasm() function, however this caused a lot of cyclic imports that I wasn't able to resolve.
  • option 2: copy qasm() logic to ControlledGate.

either way this shouldn't change the qasm code generated for the standard controlled gates (cx,ccx.. etc)

I'm trying to implement option 2. But if anyone wants to give it a shot, no problem.

@rochisha0
Copy link
Contributor

rochisha0 commented Jun 30, 2020

Can I give this a try?
@faisaldebouni

@faisaldebouni
Copy link
Contributor

sure no problem.

@1ucian0
Copy link
Member

1ucian0 commented Jul 24, 2020

Any progress @rochisha0 ?

@1ucian0 1ucian0 added the mod: qasm2 Relating to OpenQASM 2 import or export label Oct 5, 2020
@1ucian0
Copy link
Member

1ucian0 commented Oct 5, 2020

@rochisha0 is not responsive. Do you mid PRing your solution @faisaldebouni ?

@faisaldebouni
Copy link
Contributor

Sure, I will work on this right away.

@faisaldebouni
Copy link
Contributor

I need some guidance with this. I see many problems with how qasm is being generated in general. Directly fixing this issue will not give the correct qasm code.

Mainly, generating qasm definition for circuits with custom gates is not done recursively.
ex:

qc = QuantumCircuit(1)
qc.x(0)

qc2 = QuantumCircuit(1)
qc2.append(qc.to_gate(),[0])

qc3 = QuantumCircuit(1)
qc3.append(qc2.to_gate(),[0])

print(qc3.qasm())

will give:

OPENQASM 2.0;
include "qelib1.inc";
gate circuit14 q0 { circuit13 q0; }
qreg q[1];
circuit14 q[0];

However, print(qc2.qasm()) will give the correct qasm code.

UnitaryGate definition is pretty flat, so the gate will be correctly defined. But UnitaryGate.control() builds a gate with nested circuits in it's definition. So, if ControlledGate adapts the same procedure for generating the qasm code, the resulting code will be incorrect. (uses undefined gates).

One "temporary" solution is to make UnitaryGate.control() revert to gate.control() which uses add_control.py. This gives a correct, yet flat definition. Which in turn make the generated qasm code correct. I couldn't figure out why UnitaryGate defines it's own control function, although add_control.py does handle UnitaryGates correctly

So My questions are, Do I:

  • adopt how qasm generation currently works regardless?
  • make sure the controlled version of unitary gates are flat?
  • work on a more ambitious, concrete implementation, for generating a valid qasm code for this case and other cases?

@faisaldebouni
Copy link
Contributor

faisaldebouni commented Oct 6, 2020

This is mock implementation that adopts how qasm generation currently works regardless.
mock branch
By simply moving qasm() from Unitary to Gate. The generated qasm from the example provided in issue will be:

gate unitary4877971528 p0,p1 {
	circuit7_dg p0,p1;
}
unitary4877971528

@1ucian0
Copy link
Member

1ucian0 commented Oct 4, 2021

It seems to me that an arbitrary matrix should be synthesised to create a QASM definition.

(Sorry this answer this late. This felt between the cracks)

@jwoehr
Copy link
Contributor

jwoehr commented Dec 15, 2021

Still valid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mod: qasm2 Relating to OpenQASM 2 import or export
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants