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

Add RemoveFinalReset pass #11266

Merged
merged 8 commits into from
Mar 19, 2024
Merged

Add RemoveFinalReset pass #11266

merged 8 commits into from
Mar 19, 2024

Conversation

garrison
Copy link
Member

@garrison garrison commented Nov 17, 2023

Summary

This adds a RemoveFinalReset pass, which removes a Reset instruction if it comes at the end of a qubit wire. It also enables this pass at optimization level 1 and higher.

Details and comments

The new wire cutting code in the Circuit Knitting Toolbox results in circuits where there are Resets in the circuit. In the special case that no qubits are re-used later in the circuit, then all of these Resets are either at the beginning of the circuit or the end of the circuit. There is an existing pass, RemoveResetInZeroState, that removes resets at the beginning of the circuit. This pass is disabled by default in the pass managers (see #10591), as it places assumptions on the state at the beginning of the circuit. This PR creates the other transpiler pass that we need, namely one that removes resets at the end of the circuit. If nothing happens to a qubit wire after, then this removal should have no effect on the circuit. Of course, this does change the result if you then concatenate it with another circuit; I'm assuming this assumption is okay, but if not, it should be removed from the default passes. EDIT: I have removed this from the default passes in 4adc048.

Related CKT issue: Qiskit/qiskit-addon-cutting#452

@garrison garrison requested a review from a team as a code owner November 17, 2023 14:07
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Nov 17, 2023

Pull Request Test Coverage Report for Build 8347062095

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 15 of 15 (100.0%) changed or added relevant lines in 3 files are covered.
  • 13 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.3%) to 89.316%

Files with Coverage Reduction New Missed Lines %
qiskit/primitives/containers/estimator_pub.py 1 97.59%
qiskit/primitives/containers/sampler_pub.py 2 97.26%
crates/qasm2/src/lex.rs 4 92.44%
crates/qasm2/src/parse.rs 6 97.62%
Totals Coverage Status
Change from base Build 8320404144: -0.3%
Covered Lines: 59822
Relevant Lines: 66978

💛 - Coveralls

Comment on lines 32 to 36
resets = dag.op_nodes(Reset)
for reset in resets:
successor = next(dag.successors(reset))
if isinstance(successor, DAGOutNode):
dag.remove_op_node(reset)
Copy link
Member

@mtreinish mtreinish Nov 17, 2023

Choose a reason for hiding this comment

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

It'll probably be a bit more efficient to use the output_map in the dag and look at it's predecessors. This is because dag.op_nodes() iterates over all the instructions in the dag and in a typical circuit there will be far more instructions than qubits and clbits.

Suggested change
resets = dag.op_nodes(Reset)
for reset in resets:
successor = next(dag.successors(reset))
if isinstance(successor, DAGOutNode):
dag.remove_op_node(reset)
for output_node in dag.output_map.values():
if isinstance(output_node.wire, Qubit):
pred = next(dag.predecessors(output_node))
if isinstance(pred, DAGOpNode) and isinstance(pred.op, Reset):
dag.remove_op_node(pred)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in f358d46. I wonder if this could potentially be improved in the RemoveResetInZeroState pass too (I modeled my original code on this pass).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can probably make a similar change to RemoveResetInZeroState. It looks like it hasn't had any meaningful updates since it was first introduced in #2139.

garrison and others added 3 commits November 17, 2023 15:15
Suggested at Qiskit#11266 (comment)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
This pass modifies the quantum output state of the circuit, so it
should not be run by default.
mtreinish
mtreinish previously approved these changes Mar 18, 2024
@mtreinish mtreinish enabled auto-merge March 18, 2024 11:39
It's no longer run by default at any optimization level.
auto-merge was automatically disabled March 19, 2024 01:26

Head branch was pushed to by a user without write access

@garrison
Copy link
Member Author

@mtreinish: I fixed a few things; this is ready to merge now.

from qiskit.transpiler import PassManager
from qiskit.transpiler.passes import RemoveFinalReset, DAGFixedPoint
from qiskit.converters import circuit_to_dag
from qiskit.test import QiskitTestCase
from test import QiskitTestCase # pylint: disable=wrong-import-order
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, you could make pylint happy if you added a new line between the qiskit import section and this import.

@mtreinish mtreinish added this pull request to the merge queue Mar 19, 2024
Merged via the queue into Qiskit:main with commit 494a32e Mar 19, 2024
12 checks passed
@garrison garrison deleted the remove-final-reset branch April 24, 2024 00:39
@sbrandhsn sbrandhsn added the Changelog: New Feature Include in the "Added" section of the changelog label May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants