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

Improve efficiency of RemoveFinalMeasurement transpiler pass #7039

Merged
merged 5 commits into from
Sep 17, 2021

Conversation

mtreinish
Copy link
Member

Summary

The RemoveFinalMeasurement was determining which operations were at the
end of a circuit quite inefficiently. It was searching over the entire
DAG to find barrier and measurement instructions and then checking any
that were found if they're children were output nodes. While instead
it'd be much more efficient to chack the parent of each output node for
whether it's a barrier or a measurement. Additionally, the output dag
was being needless rebuilt by modifying the input and then looping over
all nodes in the dag again and copy the node to a new dag. While instead
we can just return the modified input graph. This commit makes those
changes to improve the efficiency of the pass.

Details and comments

Fixes #7038

The RemoveFinalMeasurement was determining which operations were at the
end of a circuit quite inefficiently. It was searching over the entire
DAG to find barrier and measurement instructions and then checking any
that were found if they're children were output nodes. While instead
it'd be much more efficient to chack the parent of each output node for
whether it's a barrier or a measurement. Additionally, the output dag
was being needless rebuilt by modifying the input and then looping over
all nodes in the dag again and copy the node to a new dag. While instead
we can just return the modified input graph. This commit makes those
changes to improve the efficiency of the pass.

Fixes Qiskit#7038
@mtreinish mtreinish requested a review from ewinston September 17, 2021 19:44
@mtreinish mtreinish requested a review from a team as a code owner September 17, 2021 19:44
@mtreinish
Copy link
Member Author

mtreinish commented Sep 17, 2021

Running this script:

import time
from qiskit.circuit.random import random_circuit
from qiskit.converters import circuit_to_dag

widths = [10, 100, 1000]

for width in widths:
    circuit = random_circuit(width, width, measure=True, seed=42)
    start = time.time()
    circuit.remove_final_measurements()
    stop = time.time()
    print(f"Width {width}: {stop - start}")

Running with this PR:

Width 10: 0.0017766952514648438
Width 100: 0.12475299835205078
Width 1000: 18.337160348892212

Running from main without this PR:

Width 10: 0.002696514129638672
Width 100: 0.7955341339111328
Width 1000: 1058.9532570838928

Now to just fix the test failures fixing the logic and improving the performance in idle_wires() caused fixed in mtreinish@bd96bab

@mtreinish mtreinish force-pushed the perf-remove-final-measure branch from bd96bab to 89b1acf Compare September 17, 2021 20:54
Copy link
Member

@kdk kdk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! QuantumCircuit.remove_final_measurementswould probably also benefit from a similar treatment, but that can be for another issue.

Comment on lines +800 to +803
except StopIteration as e:
raise DAGCircuitError(
"Invalid dagcircuit input node %s has no output" % self.input_map[wire]
) from e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, we don't usually guard against a malformed DAGCircuit, was there a reason to here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pylint was yelling at me locally about raising stopiteration without it. This made it happy...

@kdk kdk merged commit 1892a8f into Qiskit:main Sep 17, 2021
@mtreinish mtreinish deleted the perf-remove-final-measure branch September 17, 2021 23:54
@mtreinish
Copy link
Member Author

This actually will apply to the circuit method too, internally it just uses the transpiler pass: https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/circuit/quantumcircuit.py#L2140-L2181(it's really the only thing using the pass)

@kdk kdk added this to the 0.19 milestone Nov 15, 2021
@kdk kdk added the Changelog: None Do not include in changelog label Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RemoveFinalMeasurement scales superlinearly with gates
2 participants