`UnitaryGate.qasm()` fails

richrines1 commented 1 year ago

Environment

Qiskit Terra version: 0.24.1
Python version: 3.10
Operating system: ubuntu 22.04

What is happening?

calling UnitaryGate(<array>).qasm() fails after version 0.24.0, because it attempts to call pi_check on the unitary array. (however QuantumCircuit.qasm still works for circuits containing UnitaryGates)

How can we reproduce the issue?

import qiskit
import numpy as np

gate = qiskit.extensions.UnitaryGate(np.eye(2))
_ = gate.qasm()  # TypeError: only length-1 arrays can be converted to Python scalars (thrown from `pi_check()`)

What should happen?

gate.qasm() should return a qasm string, or otherwise should return something like None or NotImplemented instead of throwing an error

Any suggestions?

No response

SimoneGasperini commented 1 year ago

I started looking into this and I think the problem derives from the internal representation of the UnitaryGate parameters. Indeed, it looks that the created gate has only one parameter which corresponds to the entire 2d-array used to instantiate the gate object itself:

from qiskit.extensions import UnitaryGate
import numpy as np

gate = UnitaryGate(np.eye(2))
print(gate.params)

[array([[1.+0.j, 0.+0.j],
       [0.+0.j, 1.+0.j]])]

This is the reason why the pi_check function fails, since it is supposed to loop over each single "scalar" parameter one by one. Calling the qasm method on the QuantumCircuit instance works fine because the appended UnitaryGate is processed by the function _qasm2_define_custom_operation which takes care of redefining it by using the supported existing_gate_names and of making up the proper QASM call. For the identity example above we have the following:

from qiskit import QuantumCircuit

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

print(qc.qasm())

OPENQASM 2.0;
include "qelib1.inc";
gate unitary q0 { u3(0,0,0) q0; }
qreg q[1];
unitary q[0];

My suggestion would be to reimplement the qasm method for the UnitaryGate class to make it returning a more complex QASM string including both the redefinition and the invocation of the given unitary. For the above case, you should get:

print(gate.qasm())

gate unitary q0 { u3(0,0,0) q0; }
unitary

Another simpler option could be to internally call the method UnitaryGate._qasm2_decomposition every time qasm is invoked on a UnitaryGate instance:

print(gate._qasm2_decomposition().qasm())

unitary

The problem with this approach is that the unitary QASM string lacks its own definition and it will always be the same regardless of what the original unitary gate was.

jakelishman commented 1 year ago

@richrines1: I'm sorry this broke whatever workflow you had. Really, UnitaryGate.qasm was always logically questionable (tbh, the entire model of having an Instruction.qasm method with no arguments is hopelessly broken), because it was trying to do a job that can't be done without the full context of an OQ2 exporter - the output pretended that it had a new name (which couldn't easily be accessed), so accessing the definition needed a special case in everything that might handle it, which defeats the purpose of the method in the first place. The logic in it also meant that if label was set, there'd typically be duplicate-definition conflicts in the exporter, resulting in an invalid program.

I was largely hoping to deprecate and remove the entire Instruction.qasm method (and all derivatives) very shortly. As you say, the slightly reworked OQ2 exporter doesn't actually call that function if it can avoid it (and if I'd had time to completely rewrite the OQ2 exporter, I'd have removed all calls to Instruction.qasm or the similar internal-only replacement I needed to put in because of the time constraint). Can I ask your use-case for Instruction.qasm, so we can see what kind of features you need support for?

Parallel to any potential deprecation/removal, I think having Instruction.qasm return NotImplemented if it sees non-float parameters could be a sensible solution.

@SimoneGasperini: thank you for looking into this. The main issue is that UnitaryGate.qasm's broken override of Instruction.qasm was removed in 0.24 as part of a wider series of bugfixes to the OQ2 exporter. The Instruction.qasm method was never really meant to be used outside of the context of the built-in OQ2 exporter, but we didn't do a great job around that API design when it was first added. We wouldn't recommend that people use any internal-only functions (_qasm2_decomposition, in this case); they're not part of the public API, and are liable to change behaviour or be completely removed without warning.

richrines1 commented 1 year ago

@jakelishman it's no problem at all! thanks for the response

i definitely agree that having an argument-free Instuction.qasm() is a bit ill-defined as a model, and if it's intentionally being deprecated it's something we can easily work around. tbh our use case was a bit off-label: basically Instruction.qasm() turned out to be a pretty simple and quick (and usually effective, though admittedly imperfect) hash function for instructions, which was useful for speeding up processing in a few cases

jakelishman commented 1 year ago

Ah, yeah, that's pretty tricky. In general, Instruction instances are not going to be hashable, because it's a defined part of the data model that users can subclass them to have any sort of mutable state that they like, and non-unitary instructions are generally representable in OpenQASM 2, so that's not reliable either.

If you're concerned with hashing things after they've been compiled to a known basis, the situation is a lot simpler because you'll know which operations you're working with.

Qiskit / qiskit