Fix: BasisTranslator processing of nested ControlFlowOp#15875
Fix: BasisTranslator processing of nested ControlFlowOp#15875Cryoris merged 8 commits intoQiskit:mainfrom
BasisTranslator processing of nested ControlFlowOp#15875Conversation
- Remove the bool output `is_updated` from `apply_translation` helper method in `BasisTranslator`.
The removal of this argument ensures that when a translation is found it is always applied. The `is_modified` boolean output, while intended as a way of optimizing the operation of the `BasisTranslator` did not achieve much as a new `DAGCircuit would be created each time.
During the `ControlFlowOp` handling step of this method we relied on `is_updated` to determine whether to use the original `DAGCircuit` or not, which in the end would erroneously skip certain transformations in the `Operation`'s blocks.
By removing this tracking value, which was already ignored by the main `BasisTranslator` process. We end up with a more correct processing of the instructions at hand.
- Update test `test_inner_wire_map_control_op` from the `TestCircuitControlFlowOps` as better outcome is now found for said circuit.
- Inspired on [comment](Qiskit#13162 (comment)) from @jakelishman.
raynelfss
added
Changelog: Fixed
|
One or more of the following people are relevant to this code:
|
mtreinish
added
the
stable backport potential
|
thank you for sharing the PR, it clarified how control flow should be handles, great learning experience. Thanks alot just wanted to ask is it covering all nested blocks like all nested blocks are always translated to target basis in single pass |
Cryoris
left a comment
Cryoris
left a comment
There was a problem hiding this comment.
Some minor comments below otherwise LGTM, thanks Ray! I also double checked that this fixes the drawer error.
| cqc = pm.run(qc) | ||
| self.assertEqual(Operator(qc), Operator.from_circuit(cqc)) | ||
|
|
||
| def test_basis_nested_control_flow_op(self): |
There was a problem hiding this comment.
Could we add a test that uses the AerSimulator from the bug reports, too, to ensure we have the full regression test?
There was a problem hiding this comment.
I refrained from adding a test that does this basically because I'd need to change the dependency list for test to include qiskit_aer. I can add a test using a GenericBackendV2 and uses a PresetPassManager to run the transpiler pipeline. It should produce a very similar outcome.
There was a problem hiding this comment.
We have several tests using Aer, that's what the optionals package is for. E.g. something like
@unittest.skipUnless(HAS_AER, "Aer required for clifford simulation")
def test_thing(self):
from qiskit_aer import AerSimulator
# test your thingIt's fine to use it 🙂
| """Test that the gates inside ControlFlowOps land on correct qubits when transpiled""" | ||
| expected = "\n".join( | ||
| [ | ||
| " ", |
There was a problem hiding this comment.
To document this: Ray and I checked this test and it's still the same equivalence since the inside of the control flow has a global phase shift of pi to account to the sign flip here.
| control flow operations would have its blocks skip the calculated | ||
| transformations. | ||
|
|
||
| See `#13162 <https://github.com/Qiskit/qiskit/issues/13162>`__. No newline at end of file |
There was a problem hiding this comment.
If we list the fixed issues, we probably should list them all 🙂
- Add new test using a backend and the full transpiler pipeline. - Add all issues to release note. - Small language fix.
@rajnisht7 Yes, it always calculates transformations for each block recursively. The problem is that it would (in most cases) just skip the transformation entirely. Thankfully, with this fix, it behaves the way it is supposed to. |
releasenotes/notes/fix_basis_control_flow-033ecba233d5f9ed.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Cryoris
left a comment
Cryoris
left a comment
There was a problem hiding this comment.
LGTM thanks for the fix, Ray!
* Fix: `BasisTranslator` processing of `ControlFlowOp`
- Remove the bool output `is_updated` from `apply_translation` helper method in `BasisTranslator`.
The removal of this argument ensures that when a translation is found it is always applied. The `is_modified` boolean output, while intended as a way of optimizing the operation of the `BasisTranslator` did not achieve much as a new `DAGCircuit would be created each time.
During the `ControlFlowOp` handling step of this method we relied on `is_updated` to determine whether to use the original `DAGCircuit` or not, which in the end would erroneously skip certain transformations in the `Operation`'s blocks.
By removing this tracking value, which was already ignored by the main `BasisTranslator` process. We end up with a more correct processing of the instructions at hand.
- Update test `test_inner_wire_map_control_op` from the `TestCircuitControlFlowOps` as better outcome is now found for said circuit.
* Test: Add test
- Inspired on [comment](#13162 (comment)) from @jakelishman.
* Docs: Add release note
* Address review comments:
- Add new test using a backend and the full transpiler pipeline.
- Add all issues to release note.
- Small language fix.
* Lint: formatting
* Fix: Make new test use aer
* Update releasenotes/notes/fix_basis_control_flow-033ecba233d5f9ed.yaml
Co-authored-by: Julien Gacon <gaconju@gmail.com>
---------
Co-authored-by: Julien Gacon <gaconju@gmail.com>
(cherry picked from commit 9c8f4fc)
…15884) * Fix: `BasisTranslator` processing of `ControlFlowOp` - Remove the bool output `is_updated` from `apply_translation` helper method in `BasisTranslator`. The removal of this argument ensures that when a translation is found it is always applied. The `is_modified` boolean output, while intended as a way of optimizing the operation of the `BasisTranslator` did not achieve much as a new `DAGCircuit would be created each time. During the `ControlFlowOp` handling step of this method we relied on `is_updated` to determine whether to use the original `DAGCircuit` or not, which in the end would erroneously skip certain transformations in the `Operation`'s blocks. By removing this tracking value, which was already ignored by the main `BasisTranslator` process. We end up with a more correct processing of the instructions at hand. - Update test `test_inner_wire_map_control_op` from the `TestCircuitControlFlowOps` as better outcome is now found for said circuit. * Test: Add test - Inspired on [comment](#13162 (comment)) from @jakelishman. * Docs: Add release note * Address review comments: - Add new test using a backend and the full transpiler pipeline. - Add all issues to release note. - Small language fix. * Lint: formatting * Fix: Make new test use aer * Update releasenotes/notes/fix_basis_control_flow-033ecba233d5f9ed.yaml --------- (cherry picked from commit 9c8f4fc) Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com> Co-authored-by: Julien Gacon <gaconju@gmail.com>
6 participants
Summary
Inspired by #15797
The following PR fixes a long standing bug in
BasisTranslatorby allowing all calculated transformations to be applied within aControlFlowOp.Details and comments
Fixes #15734
Fixes #14025
Fixes #13162
is_updatedfromapply_translationhelper method inBasisTranslator. The removal of this argument ensures that when a translation is found it is always applied.is_modifiedboolean output, while intended as a way of optimizing the operation of theBasisTranslatordid not achieve much as a newDAGCircuitwould be created each time.ControlFlowOphandling step of this method we relied onis_updatedto determine whether to use the originalDAGCircuitor not, which in the end would erroneously skip certain transformations in theOperation's blocks.BasisTranslatorprocess. We end up with a more correct processing of the instructions at hand.test_inner_wire_map_control_opfrom theTestCircuitControlFlowOpsas better outcome is now found for said circuit.