Speed up DAGCircuit.reverse_ops() #10448
Conversation
This commit significantly improves the runtime performance of the DAGCircuit.reverse_ops() method. Previously, the method worked by converting the DAGCircuit to a QuantumCircuit and using QuantumCircuit's reverse_ops() method and then converting back to a DAGCircuit. This has significant overhead and in there was an inline TODO comment saying to speed it up. This commit rewrites the internals to build a shallow copy of the DAGCircuit and then reversing the edge directionality of the inner rustworkx graph using rustworkx's graph reverse method. This ends up being significantly faster in local testing running reverse_ops() on a 1024 qubit and depth random circuit the time it took reverse_ops() to run went from ~17 seconds with current main to ~0.05 with this change. The only tradeoff here is that the output DAGCircuit from the method will now have shared references to the original DAG, while in the previous implementation the output DAGCircuit was effectively a deep copy with no shared references.
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5599437673
💛 - Coveralls |
There was a problem hiding this comment.
This technically doesn't have the same behaviour for compound instructions as QuantumCircuit.reverse_ops, so this is a behavioural change for them. The QuantumCircuit version recursively calls Instruction.reverse_ops on all the inner instructions, which reverses their _definition attribute if it exists (I'm not a fan of that in modern Terra, though I think it did use to make a bit more sense back in the #1816 days).
I think for all of Terra's uses of this function, that's not actually going to matter, but it's worth considering what we want to do about it.
| The output reversed :class:`~.DAGCircuit` object will have shared | ||
| references for all operations and qubits to this dag. |
There was a problem hiding this comment.
Qubits are immutable, so it's probably not worth mentioning them. If we're worried about ownership here, it could be worth adding a copy_operations flag like we've got for circuit_to_dag and dag_to_circuit, and replace the copy.copy with a copy.deepcopy based on that. I'm not certain if it's worth it, though.
There was a problem hiding this comment.
My first attempt at this locally was using a deepcopy instead. The deepcopy on its own ended up being slower than the previous implementation of the method. I figured this was an ok tradeoff because the ownership here wasn't clearly established before. But I don't feel super strongly about it either.
|
fwiw, the only thing preventing me from approving this is us needing to resolve how to handle the potential behaviour change wrt compound instructions I mentioned in the prior review comment. |
Pull Request Test Coverage Report for Build 6224818743
💛 - Coveralls |
|
Closing this because it's not relevant anymore and we'd have to write it in rust now if we wanted to do this. |
Summary
This commit significantly improves the runtime performance of the DAGCircuit.reverse_ops() method. Previously, the method worked by converting the DAGCircuit to a QuantumCircuit and using QuantumCircuit's reverse_ops() method and then converting back to a DAGCircuit. This has significant overhead and in there was an inline TODO comment saying to speed it up. This commit rewrites the internals to build a shallow copy of the DAGCircuit and then reversing the edge directionality of the inner rustworkx graph using rustworkx's graph reverse method. This ends up being significantly faster in local testing running reverse_ops() on a 1024 qubit and depth random circuit the time it took reverse_ops() to run went from ~17 seconds with current main version to ~0.05 with this change. The only tradeoff here is that the output DAGCircuit from the method will now have shared references to the original DAG, while in the previous implementation the output DAGCircuit was effectively a deep copy with no shared references.
Details and comments