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

Fix LocalNoisePass for circuit returns from noise func #1455

Merged
merged 5 commits into from
Feb 8, 2022

Conversation

chriseclectic
Copy link
Member

Summary

Fixes #1447

IF the local noise pass function returns a QuantumCircuit instead of an instruction or operator this composes the circuit into the DAG so that it is unrolled, rather than added as an opaque circuit instruction.

Details and comments

@chriseclectic chriseclectic added this to the Aer 0.10.3 milestone Feb 7, 2022
IF the local noise pass function returns a QuantumCircuit instead of an instruction or operator this composes the circuit into the DAG so that it is unrolled, rather than added as an opaque circuit instruction.
This added check breaks current handling of adding noise to measure instructions.
mtreinish
mtreinish previously approved these changes Feb 7, 2022
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, seems straightforward enough one small comment typo inline. It'd be good to get @itoko to review this too as he's more familiar with this pass though.

Also something I couldn't think of a good place to ask inline, but is there a scenario where new_dag contains a single op node? If so it'd probably be much more efficient to just do substitute_node(node, new_op, inplace=True) instead of substitute_node_with_dag() as it's just a reference update and doesn't need to traverse the new dag or deal with any of those complexities. (but that should be a follow-up PR optimization if it can be done, and not something I'd want to backport either)

@chriseclectic chriseclectic requested a review from itoko February 7, 2022 22:43
@chriseclectic
Copy link
Member Author

@mtreinish I think it is usually at least 2 nodes (the original + the error) unless the pass is being run with replace=True.

If replace is True then in principle it could be a single node substitution (depending on function return being a single N-qubit instruction/op vs a quantum circuit). If it has a major effect on performance it's something that could be handled as a special case, and we could exploit it with pre-made passes by replacing gates with a quantum channel that is ideal.compose(error) rather than appending just the error after the ideal gate.

@chriseclectic chriseclectic added Changelog: Bugfix Include in the Fixed section of the changelog stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable labels Feb 7, 2022
itoko
itoko previously approved these changes Feb 8, 2022
Copy link
Contributor

@itoko itoko left a comment

Choose a reason for hiding this comment

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

LGTM, I couldn't think of any better way to fix. I just left one minor suggestion to make the test a bit straightforward.

@mtreinish
Copy link
Member

If replace is True then in principle it could be a single node substitution (depending on function return being a single N-qubit instruction/op vs a quantum circuit). If it has a major effect on performance it's something that could be handled as a special case, and we could exploit it with pre-made passes by replacing gates with a quantum channel that is ideal.compose(error) rather than appending just the error after the ideal gate.

For a single node replacement substitute_node is about 10-15x faster with inplace=True (and 5-7x with in_place=False) than substitute_node_with_dag(). That's also ignoring construction time to create the subcircuit equivalent dag to replace the node with. I made a little benchmark script to test this:

import time

from qiskit.converters import circuit_to_dag
from qiskit import QuantumCircuit
from qiskit.circuit.library import U2Gate

qc = QuantumCircuit(2)
qc.h(0)
qc.cx(0, 1)
qc.measure_all()
# Build dags for benchmarking
dag = circuit_to_dag(qc)
dag_two = circuit_to_dag(qc)
# equiv replacement for sub node with dag
equiv_h = QuantumCircuit(1)
equiv_h.u2(0, 3.14159, 0)
equiv_dag = circuit_to_dag(equiv_h)
# sub in place
dag_node_to_replace = dag.op_nodes()[0]
equiv_node = U2Gate(0, 3.14159)
start = time.time()
dag.substitute_node(dag_node_to_replace, equiv_node, inplace=False)
stop = time.time()
print(f"substitute_node: {stop - start}")
# sub with dag
dag_node_to_replace = dag_two.op_nodes()[0]
start = time.time()
dag_two.substitute_node_with_dag(dag_node_to_replace, equiv_dag)
stop = time.time()
print(f"substitute_node_with_dag: {stop - start}")

But like I said earlier it's not really something we should worry about for this PR or the bugfix release. Just a potential optimization in the future.

@chriseclectic chriseclectic merged commit 6461df8 into Qiskit:main Feb 8, 2022
hhorii pushed a commit that referenced this pull request Feb 9, 2022
If the local noise pass function returns a QuantumCircuit instead of an instruction or operator this composes the circuit into the DAG so that it is unrolled, rather than added as an opaque circuit instruction.

Co-authored-by: Matthew Treinish <[email protected]>
Co-authored-by: Toshinari Itoko <[email protected]>
hhorii pushed a commit to hhorii/qiskit-aer that referenced this pull request Feb 9, 2022
If the local noise pass function returns a QuantumCircuit instead of an instruction or operator this composes the circuit into the DAG so that it is unrolled, rather than added as an opaque circuit instruction.

Co-authored-by: Matthew Treinish <[email protected]>
Co-authored-by: Toshinari Itoko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the Fixed section of the changelog stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable
Projects
None yet
3 participants