-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 InverseCancellation
to run in classical blocks
#13454
Fix InverseCancellation
to run in classical blocks
#13454
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 12038891561Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
Can you also add some tests? You can probably just more or less copy the tests we had for CXCancellation
's control flow handling into test_inverse_cancellation.py
.
qiskit/test/python/transpiler/test_cx_cancellation.py
Lines 146 to 199 in 94cea41
def test_if_else(self): | |
"""Test that the pass recurses in a simple if-else.""" | |
with self.assertWarns(DeprecationWarning): | |
pass_ = CXCancellation() | |
inner_test = QuantumCircuit(4, 1) | |
inner_test.cx(0, 1) | |
inner_test.cx(0, 1) | |
inner_test.cx(2, 3) | |
inner_expected = QuantumCircuit(4, 1) | |
inner_expected.cx(2, 3) | |
test = QuantumCircuit(4, 1) | |
test.h(0) | |
test.measure(0, 0) | |
test.if_else((0, True), inner_test.copy(), inner_test.copy(), range(4), [0]) | |
expected = QuantumCircuit(4, 1) | |
expected.h(0) | |
expected.measure(0, 0) | |
expected.if_else((0, True), inner_expected, inner_expected, range(4), [0]) | |
self.assertEqual(pass_(test), expected) | |
def test_nested_control_flow(self): | |
"""Test that collection recurses into nested control flow.""" | |
with self.assertWarns(DeprecationWarning): | |
pass_ = CXCancellation() | |
qubits = [Qubit() for _ in [None] * 4] | |
clbit = Clbit() | |
inner_test = QuantumCircuit(qubits, [clbit]) | |
inner_test.cx(0, 1) | |
inner_test.cx(0, 1) | |
inner_test.cx(2, 3) | |
inner_expected = QuantumCircuit(qubits, [clbit]) | |
inner_expected.cx(2, 3) | |
true_body = QuantumCircuit(qubits, [clbit]) | |
true_body.while_loop((clbit, True), inner_test.copy(), [0, 1, 2, 3], [0]) | |
test = QuantumCircuit(qubits, [clbit]) | |
test.for_loop(range(2), None, inner_test.copy(), [0, 1, 2, 3], [0]) | |
test.if_else((clbit, True), true_body, None, [0, 1, 2, 3], [0]) | |
expected_if_body = QuantumCircuit(qubits, [clbit]) | |
expected_if_body.while_loop((clbit, True), inner_expected, [0, 1, 2, 3], [0]) | |
expected = QuantumCircuit(qubits, [clbit]) | |
expected.for_loop(range(2), None, inner_expected, [0, 1, 2, 3], [0]) | |
expected.if_else((clbit, True), expected_if_body, None, [0, 1, 2, 3], [0]) | |
self.assertEqual(pass_(test), expected) |
Done! Please kindly review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code and test LGTM, thanks for doing this! Before we merge it though do you mind adding a release note to document this fix was included in the next release? The process to do this is documented here: https://github.com/Qiskit/qiskit/blob/main/CONTRIBUTING.md#release-notes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for adding the release note so quickly.
@@ -0,0 +1,3 @@ | |||
fixes: | |||
- | | |||
The transpilation pass :class`.InverseCancellation` now runs inside of flow controlled blocks. Previously, it ignores the pairs of gates in classical blocks that can be cancelled. Refer to `#13437 <https://github.com/Qiskit/qiskit/issues/13437>` for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably word this as "recurse into control flow blocks in the circuit". But we can also do this as part of the release prep PR #13447
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion! Is there a way that I can still fix this, either in this PR or the release prep PR #13447?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have a suggested alternative wording you can let @raynelfss know, I asked him to just update the text as part of the other release note editing he's working on in #13447.
In general we're not too picky about the exact release note content in PR reviews and as there are always a fair amount of edits needed before we can publish them. So it's simple for whoever is driving a given release to just copy-edit the text of a release note all at once in that final PR for a release.
* Fix an issue that `InverseCancellation` does not run in classical blocks (#13437) * Add test functions for pass running in classical blocks. * Reformat test function file * Add a release note (cherry picked from commit 6025b7c) Co-authored-by: haimeng-zhang <33587226+haimeng-zhang@users.noreply.github.com>
@Mergifyio backport stable/1.4 |
❌ No backport have been created
GitHub error: |
Fix issue #13437.
Summary
The
InverseCancellation
pass cancels pairs of gates that are inverses of each other; it now behaves as expected when running inside classical blocks of a control flow.Details and comments
This PR fixed the bug reported in #13437 by adding the decorator
@control_flow.trivial_recurse
so that the pass can be iterated over all control-flow nodes.