Fix QI modifying input transpiled circuit on execute #7484

manoelmarques · 2022-01-05T20:49:38Z

Summary

Details and comments

When a an instance or array of transpiled circuits is passed to the method execute in QuantumInstance, it needs to be changed to array if it is an instance and if it is an array, it needs to be copied so that the input array doesn't get modified in place. For non transpiled circuits, all works because the method transpile is called and it always returns a new array.

coveralls · 2022-01-05T21:18:25Z

Pull Request Test Coverage Report for Build 1722989690

3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.0008%) to 83.15%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/utils/quantum_instance.py	3	4	75.0%

Totals
Change from base Build 1720986032:	-0.0008%
Covered Lines:	52001
Relevant Lines:	62539

💛 - Coveralls

woodsp-ibm · 2022-01-06T15:24:38Z

qiskit/utils/quantum_instance.py

-        if not had_transpiled:
+        if had_transpiled:
+            if isinstance(circuits, list):
+                circuits = circuits.copy()


Just a minor thing - I wonder whether it might be an idea to state why the copy is done here in a comment for future reference so that the reason is know and no-one might be tempted to remove the copy thinking they are improving performance. I guess the copy is only needed when we are doing measurement mitigation that will add to the list of circuits being executed. Since this is a shallow copy of the list I guess it should be fine having this done all the time and not dependent on when we know we would add extra ccts.

I will add a comment. Yes, currently the error is just when using measurement mitigation but who knows someone could add code to change the input array somewhere else in the future. The method should not be changing the input without mentioning it in the documentation in any situation.

If this does only a shallow copy, is it what we want? If we go on modifying the shallow copied circuits list, then the original objects will also be changed, right?

from qiskit.circuit.library import EfficientSU2 original = EfficientSU2(2, reps=1) circuits = [original] circuits = circuits.copy() circuits[0].x(0) print(original.draw()) # has a new X gate

The QI code never changes the original circuit. In fact QI expects it to be an array and was changing the input array in place, not the contained circuits, in the mitigation part. There is only the need to make a shallow copy to avoid it. A deep copy is overkill and would slow the code.
Making just a shallow copy was intentional.

If you passed a non transpiled circuit or a list of them, there was no problem because it was being transpiled and
a new list of transpiled circuits created. If you passed a transpiled circuit, nothing was being done and the rest of the code was treating it as a list. So there were two things fixed for transpiled circuit: create a list if it is just a circuit and make a shallow copy if it is a list. I am surprised nobody had a problem before. I guess nobody ever sent a single transpiled circuit to QI before.

There is a comment in the code as per Steve's request above.

Ah, ok now I understand. Thanks! 🙂

Cryoris · 2022-01-11T08:13:49Z

test/python/algorithms/test_measure_error_mitigation.py

@@ -332,7 +332,7 @@ def test_measurement_error_mitigation_with_vqe_ignis(self, config):

    @unittest.skipUnless(HAS_AER, "qiskit-aer is required for this test")
    @unittest.skipUnless(HAS_IGNIS, "qiskit-ignis is required to run this test")
-    def test_callibration_results(self):
+    def test_calibration(self):


Does this test cover the new changes? If yes, why? 🙂

I added a transpiled circuit input to this method to test the change in Quantum Instance. Since I fixed the method name spelling, I made it a shorter name.

Now this method went back to the way it used to be and a new unit test method was created for this PR.

Cryoris · 2022-01-11T08:15:39Z

qiskit/utils/quantum_instance.py

-        if not had_transpiled:
+        if had_transpiled:
+            if isinstance(circuits, list):
+                circuits = circuits.copy()


If this does only a shallow copy, is it what we want? If we go on modifying the shallow copied circuits list, then the original objects will also be changed, right?

from qiskit.circuit.library import EfficientSU2 original = EfficientSU2(2, reps=1) circuits = [original] circuits = circuits.copy() circuits[0].x(0) print(original.draw()) # has a new X gate

manoelmarques · 2022-01-11T14:31:14Z

The QI code never changes the original circuit. In fact QI expects it to be an array and was changing the array in place, not the contained circuits, in the mitigation part. There is only the need to make a shallow copy to avoid it. A deep copy is overkill and would slow the code.
Making just a shallow copy was intentional.

If you passed a non transpiled circuit or a list of them, there was no problem because it was being transpiled and
a new list of transpiled circuits created. If you passed a transpiled circuit, nothing was being done and the rest of the code was treating it as a list. So there were two things fixed for transpiled circuit: create a list if it is just a circuit and make a shallow copy if it is a list. I am surprised nobody had a problem before. I guess nobody ever sent a single transpiled circuit to QI before.

jakelishman

The shallow copy is the right thing here - we need to be better across Qiskit about setting contracts for what our functions do, and not just reaching for copy.deepcopy because it's easy. QuantumInstance.execute doesn't need to modify its circuit arguments and never should, so it only needs to shallow copy.

releasenotes/notes/fix-qi-transpiled-8df449529bf6d9a2.yaml

test/python/algorithms/test_measure_error_mitigation.py

woodsp-ibm · 2022-01-12T16:12:17Z

test/python/algorithms/test_measure_error_mitigation.py

@@ -367,6 +367,34 @@ def test_callibration_results(self):
            counts_array[0], counts_array[1], msg="Counts different with/without fitter."
        )

+    @unittest.skipUnless(HAS_AER, "qiskit-aer is required for this test")
+    @unittest.skipUnless(HAS_IGNIS, "qiskit-ignis is required to run this test")


We should not need ignis anymore right so this skip can be removed as ignis is now deprecated and for now the mitigation classes that are used were copied until Terra here until such time as this is all updated with the new mitigation logic.

I removed the Ignis dependency from this new unit test method

jakelishman

Sorry, I'd lost that this was waiting on me.

* Fix QI modifying input transpiled circuit on execute * Add comments * Add separate regression test * Remove Ignis dependency in the newly created test method Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit d662fbb)

* Fix QI modifying input transpiled circuit on execute * Add comments * Add separate regression test * Remove Ignis dependency in the newly created test method Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit d662fbb) Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>

* Fix QI modifying input transpiled circuit on execute * Add comments * Add separate regression test * Remove Ignis dependency in the newly created test method Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

) * Fix QI modifying input transpiled circuit on execute * Add comments * Add separate regression test * Remove Ignis dependency in the newly created test method Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Fix QI modifying input transpiled circuit on execute

7e01cff

manoelmarques added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog mod: algorithms Related to the Algorithms module labels Jan 5, 2022

manoelmarques requested a review from woodsp-ibm January 5, 2022 20:49

manoelmarques self-assigned this Jan 5, 2022

manoelmarques requested a review from a team as a code owner January 5, 2022 20:49

woodsp-ibm reviewed Jan 6, 2022

View reviewed changes

Add comments

de6b50b

manoelmarques requested a review from woodsp-ibm January 6, 2022 17:05

woodsp-ibm previously approved these changes Jan 6, 2022

View reviewed changes

Merge branch 'main' into qi

c636cdf

manoelmarques added the automerge label Jan 6, 2022

Merge branch 'main' into qi

fbe5f57

manoelmarques removed the automerge label Jan 7, 2022

Merge branch 'main' into qi

c4371a3

Cryoris reviewed Jan 11, 2022

View reviewed changes

manoelmarques requested a review from Cryoris January 11, 2022 14:31

jakelishman reviewed Jan 11, 2022

View reviewed changes

releasenotes/notes/fix-qi-transpiled-8df449529bf6d9a2.yaml Outdated Show resolved Hide resolved

test/python/algorithms/test_measure_error_mitigation.py Outdated Show resolved Hide resolved

manoelmarques and others added 3 commits January 12, 2022 09:00

Merge branch 'main' into qi

4d86724

Merge branch 'main' into qi

8168d0e

Add separate regression test

691191d

manoelmarques dismissed woodsp-ibm’s stale review via 691191d January 12, 2022 14:55

woodsp-ibm reviewed Jan 12, 2022

View reviewed changes

Remove Ignis dependency in the newly created test method

4fa4ff7

manoelmarques force-pushed the qi branch from e4b4039 to 4fa4ff7 Compare January 12, 2022 16:55

manoelmarques requested review from woodsp-ibm and jakelishman January 12, 2022 18:29

woodsp-ibm approved these changes Jan 12, 2022

View reviewed changes

jakelishman approved these changes Jan 20, 2022

View reviewed changes

jakelishman added the automerge label Jan 20, 2022

jakelishman added this to the 0.19.2 milestone Jan 20, 2022

Merge branch 'main' into qi

6414db4

mergify bot merged commit d662fbb into Qiskit:main Jan 20, 2022

mergify bot mentioned this pull request Jan 20, 2022

Fix QI modifying input transpiled circuit on execute (backport #7484) #7547

Merged

manoelmarques deleted the qi branch January 20, 2022 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix QI modifying input transpiled circuit on execute #7484

Fix QI modifying input transpiled circuit on execute #7484

manoelmarques commented Jan 5, 2022

coveralls commented Jan 5, 2022 •

edited

Loading

woodsp-ibm Jan 6, 2022

manoelmarques Jan 6, 2022

Cryoris Jan 11, 2022

manoelmarques Jan 11, 2022 •

edited

Loading

Cryoris Jan 12, 2022

Cryoris Jan 11, 2022

manoelmarques Jan 11, 2022 •

edited

Loading

manoelmarques Jan 12, 2022

Cryoris Jan 11, 2022

manoelmarques commented Jan 11, 2022 •

edited

Loading

jakelishman left a comment

woodsp-ibm Jan 12, 2022

manoelmarques Jan 12, 2022

jakelishman left a comment

Fix QI modifying input transpiled circuit on execute #7484

Fix QI modifying input transpiled circuit on execute #7484

Conversation

manoelmarques commented Jan 5, 2022

Summary

Details and comments

coveralls commented Jan 5, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1722989690

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoelmarques Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoelmarques Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manoelmarques commented Jan 11, 2022 • edited Loading

jakelishman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

coveralls commented Jan 5, 2022 •

edited

Loading

manoelmarques Jan 11, 2022 •

edited

Loading

manoelmarques Jan 11, 2022 •

edited

Loading

manoelmarques commented Jan 11, 2022 •

edited

Loading