Add method has_control_flow_op() to QuantumCircuit
#13936
Conversation
This method was added to fix a performance regression introduced by Qiskit#13921. This check should be quicker than the multiple calls to `isinstance`.
|
One or more of the following people are relevant to this code:
|
|
I'm not sure we need to do this beyond the feature-freeze deadline. There's already no need to do all the any(instruction.is_control_flow() for instruction in circuit.data) |
Pull Request Test Coverage Report for Build 15312306687Warning: 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
💛 - Coveralls |
- Add `has_control_flow_op` to autogenerate docs.
There was a problem hiding this comment.
Thanks for tackling this. I left a few comments inline. The biggest issue @jakelishman already pointed out that this shouldn't be a new public method for 2.0 so close to the release. There are paths to fixing the performance issue without adding a new method, we can add the public api in 2.1 though.
|
In this interest of getting a fix for the regression included in 2.0 I pushed up: #13942 and moved this to 2.1. We should definitely do this after 2.0 is released. |
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
- One test case with a regular control flow operation and another with a custom one.
| @@ -1550,6 +1551,82 @@ def instructions(): | |||
| self.assertEqual(circuit, expected) | |||
| self.assertEqual(circuit.name, "test") | |||
|
|
|||
| def test_circuit_has_control_flow_op(self): | |||
There was a problem hiding this comment.
I would expand this to cover a circuit with each control flow op type and make sure it's detected. The set of defined operations is here: https://github.com/Qiskit/qiskit/blob/main/qiskit/circuit/controlflow/__init__.py#L28
There was a problem hiding this comment.
Addressed in 98c2e2c
- Implement a combinatorics based test for control flow operations.
* Add: Add method has_control_flow_op(), to `QuantumCircuit` This method was added to fix a performance regression introduced by Qiskit#13921. This check should be quicker than the multiple calls to `isinstance`. * Lint: Fix formatting * Lint: Removed unused import and f-string * Docs: Add release note - Add `has_control_flow_op` to autogenerate docs. * Fix: Addressing review comments Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Docs: Fix formatting issue in `QuantumCircuit` docstring. * Lint: Fix formatting bug in `QuantumCircuit`. * Test: Add testcases - One test case with a regular control flow operation and another with a custom one. * Test: Remove unsupported test and extend original test. - Implement a combinatorics based test for control flow operations. * Fix: Import order in `test_circuit_operations` * Update test/python/circuit/test_circuit_operations.py --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
This method was added to fix a performance regression introduced by #13921. This check should be quicker than the multiple calls to
isinstance().Details and comments
This new method for
QuantumCircuitperforms a fast path to determine if a circuit has a control flow operation present within. This is performed by usingQuantumCircuit.count_ops()and checking if any of the names there belong to aControlFlowOp.Additionally, this method fixes an unintentional performance regression caused by #13921.