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
5.26k stars 2.37k forks source link

QuantumCircuitData.sort() not working #12781

Closed iyanmv closed 4 months ago

iyanmv commented 4 months ago

Environment

What is happening?

I need to obtain the instructions of a quantum circuit (QuantumCircuit or DAGCircuit) in a deterministic way. Checking at the source code I noticed the public method QuantumCircuitData.sort() but this doesn't seem to work.

How can we reproduce the issue?

from qiskit.circuit import QuantumCircuit

qc = QuantumCircuit(3)
qc.h(0)
qc.cx(0, range(1, 3))
qc.data.sort()

I get the following error:

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[55], line 4
      2 qc.h(0)
      3 qc.cx(0, range(1, 3))
----> 4 qc.data.sort()

File [/opt/conda/lib/python3.11/site-packages/qiskit/circuit/quantumcircuitdata.py:131](https://gpuhub.labservices.ch/opt/conda/lib/python3.11/site-packages/qiskit/circuit/quantumcircuitdata.py#line=130), in QuantumCircuitData.sort(self, *args, **kwargs)
    129 """In-place stable sort. Accepts arguments of list.sort."""
    130 data = list(self._circuit._data)
--> 131 data.sort(*args, **kwargs)
    132 self._circuit._data.clear()
    133 self._circuit._data.reserve(len(data))

TypeError: '<' not supported between instances of 'qiskit._accelerate.circuit.CircuitInstruction' and 'qiskit._accelerate.circuit.CircuitInstruction'

What should happen?

Return a sorted QuantumCircuitData object.

Any suggestions?

No response

jakelishman commented 4 months ago

This is expected behaviour - QuantumCircuitData is a shim to get something that looks and behaves exactly like a Python list as a backwards compatibility measure (it used to just be a raw list). The items of the list don't have an ordering, though, so list.sort or the builtin sorted function are throwing the correct error here.

I'm not quite sure what you mean by "obtain the instructions in a deterministic way". iter(QuantumCircuit), DAGCircuit.op_nodes() and DAGCircuit.topological_op_nodes() are all deterministic.

iyanmv commented 4 months ago

I'm not quite sure what you mean by "obtain the instructions in a deterministic way". iter(QuantumCircuit), DAGCircuit.op_nodes() and DAGCircuit.topological_op_nodes() are all deterministic.

Are they? I had an issue when comparing DAGCircuit in between transpilation stages, and I thought it was because some operations were not in the same order. But I will double check. And if they are not, I will provide a small example.

jakelishman commented 4 months ago

Different transpiler stages might remove and reinsert the operations (potentially in different places), which could cause the modified DAGCircuit to have a different iteration order even if the topological ordering stays the same (topological orders are not generally unique), but for a given DAGCircuit without modification, topological_op_nodes should always return the same order.

Either way: what kind of determinism are you looking for where the actual order of the operations doesn't matter?

iyanmv commented 4 months ago

Different transpiler stages might remove and reinsert the operations (potentially in different places), which could cause the modified DAGCircuit to have a different iteration order even if the topological ordering stays the same (topological orders are not generally unique), but for a given DAGCircuit without modification, topological_op_nodes should always return the same order.

I know, but to test this I wrote a custom plugin for the scheduling stage. This is the last transpilation stage, so no additional operations should be added/removed/moved, right? Within the run() of the TransformationPass I wrote, if I do, for example, the following [op for op in dag.op_nodes() if op.name == "rz"] is not the same list as if I do the same on the ISA circuit returned by the pass manager.

Either way: what kind of determinism are you looking for where the actual order of the operations doesn't matter?

Well, that's a different topic. At the moment let's say that I do care about the exact order of the operations, even if that leads to equivalent circuits.

iyanmv commented 4 months ago

I will check about topological_op_nodes, I was using op_nodes in my example.

iyanmv commented 4 months ago

Okay, yes, topological_op_nodes() seems to guarantee that. So something like [node for node in dag.topological_nodes() if isinstance(node, DAGOpNode) and node.op.name == "rz"] is deterministic in the way I need. Thanks for the help 😉

Btw, I'm waiting for an invitation for the Qiskit slack. I guess in the future I can make less noise here in the repos and ask questions directly there with other qiskit users.

jakelishman commented 4 months ago

Ah ok, I think the trick here then is that op_nodes is just iterating through the nodes in a deterministic but not-usually-helpful order. op_nodes is going to depend a lot on insertion order to the DAG, and substitutions will cause that to be all super weird. topological_op_nodes will always make the operations come out in an order where each item is strictly not depended on by any earlier item. That's probably enough for you.

I'm not sure what's going on with the Qiskit slack, sorry. I sent the email in your GitHub profile an invitation - hopefully that works for you.