Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
4.83k stars 2.29k forks source link

ControlFlowOp with parameterized body is not parameterized #12624

Open nkanazawa1989 opened 1 week ago

nkanazawa1989 commented 1 week ago

What should we add?

When I test parameterization of below control flow, I would expect it is parameterized.

from qiskit.circuit import QuantumCircuit, Qubit, Clbit, Parameter

p1 = Parameter("P1")
p2 = Parameter("P2")

bits = [Qubit(), Qubit(), Clbit()]
qc = QuantumCircuit(bits)
qc.sx(0)
qc.measure(0, 0)
with qc.if_test((bits[2], 0)) as else_:
    qc.rx(p1, 0)
with else_:
    qc.rx(p2, 0)

qc._data[2].operation.is_parameterized()
False

But I get False because is_parameterized function is defined in Instruction and it only tests against ParameterExpression type. ControlFlowOp must overwrite the method to recursively check parameterization of its body.

I'm not sure if this is expected behavior. If we change current behavior this is indeed an API break and we should be very careful.

jakelishman commented 1 week ago

I'm not sure what, if any, the intended behaviour was, really - I had forgotten this method even existed.

I'm not 100% certain what the best path is here. I very roughly lean towards leaving it as-is and documenting the behaviour clearly, because as you say, it'd be a breaking change, and the problem has been around ever since control flow was first introduced. That said, I forgot the method even existed, so I clearly don't really know how it's being used. If there's a clear compelling use case for making it have the new behaviour, we can go that way.

nkanazawa1989 commented 1 week ago

I'm fine with only updating documentation (or maybe deprecate and overwrite with NotImplemented). Sometime this can be useful for payload validation, e.g. checking if all unbound parameters are operands of allowed gate type. However, this method is just a syntactic sugar of

inst.operation.is_parameterized() == any(isinstance(p, ParameterExpression) for p in inst.operation.params)

and I don't think the readability of the code is improved so much. But if we write validation logic with this method, the method is called very frequently, and if we implement this with Rust, probably we can get some performance gain.