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.

Sign up for GitHub

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 QI modifying input transpiled circuit on execute #7484

Merged
merged 10 commits into from
Jan 20, 2022
Merged

Fix QI modifying input transpiled circuit on execute #7484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7e01cff
Fix QI modifying input transpiled circuit on execute
manoelmarques Jan 5, 2022
de6b50b
Add comments
manoelmarques Jan 6, 2022
c636cdf
Merge branch 'main' into qi
manoelmarques Jan 6, 2022
fbe5f57
Merge branch 'main' into qi
mergify[bot] Jan 7, 2022
c4371a3
Merge branch 'main' into qi
manoelmarques Jan 10, 2022
4d86724
Merge branch 'main' into qi
manoelmarques Jan 12, 2022
8168d0e
Merge branch 'main' into qi
manoelmarques Jan 12, 2022
691191d
Add separate regression test
manoelmarques Jan 12, 2022
4fa4ff7
Remove Ignis dependency in the newly created test method
manoelmarques Jan 12, 2022
6414db4
Merge branch 'main' into qi
mergify[bot] Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions qiskit/utils/quantum_instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This code is part of Qiskit.
#
# (C) Copyright IBM 2018, 2021.
# (C) Copyright IBM 2018, 2022.
#
# This code is licensed under the Apache License, Version 2.0. You may
# obtain a copy of this license in the LICENSE.txt file in the root directory
Expand Down Expand Up @@ -491,8 +491,18 @@ def execute(self, circuits, had_transpiled: bool = False):
build_measurement_error_mitigation_qobj,
)

# maybe compile
if not had_transpiled:
if had_transpiled:
# Convert to a list or make a copy.
# The measurement mitigation logic expects a list and
# may change it in place. This makes sure that logic works
# and any future logic that may change the input.
# It also makes the code easier: it will always deal with a list.
if isinstance(circuits, list):
circuits = circuits.copy()
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@manoelmarques manoelmarques Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok now I understand. Thanks! 🙂

else:
circuits = [circuits]
else:
# transpile here, the method always returns a copied list
circuits = self.transpile(circuits)

from qiskit.providers import BackendV1
Expand Down
6 changes: 6 additions & 0 deletions releasenotes/notes/fix-qi-transpiled-8df449529bf6d9a2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
fixes:
- |
Fix :meth:`~qiskit.utils.QuantumInstance.execute` method in the
:class:`~qiskit.utils.QuantumInstance` to not modify the transpiled input circuit array.
Refer to the discussion in `#7449 <https://github.com/Qiskit/qiskit-terra/issues/7449>`__.
30 changes: 28 additions & 2 deletions test/python/algorithms/test_measure_error_mitigation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This code is part of Qiskit.
#
# (C) Copyright IBM 2019, 2021.
# (C) Copyright IBM 2019, 2022.
#
# This code is licensed under the Apache License, Version 2.0. You may
# obtain a copy of this license in the LICENSE.txt file in the root directory
Expand Down Expand Up @@ -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_results(self):
"""check that results counts are the same with/without error mitigation"""
algorithm_globals.random_seed = 1679
np.random.seed(algorithm_globals.random_seed)
Expand Down Expand Up @@ -367,6 +367,32 @@ 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")
def test_circuit_modified(self):
"""tests that circuits don't get modified on QI execute with error mitigation
as per issue #7449
"""
algorithm_globals.random_seed = 1679
np.random.seed(algorithm_globals.random_seed)
circuit = QuantumCircuit(1)
circuit.x(0)
circuit.measure_all()

qi = QuantumInstance(
Aer.get_backend("aer_simulator"),
seed_simulator=algorithm_globals.random_seed,
seed_transpiler=algorithm_globals.random_seed,
shots=1024,
measurement_error_mitigation_cls=CompleteMeasFitter,
)
# The error happens on transpiled circuits since "execute" was changing the input array
# Non transpiled circuits didn't have a problem because a new transpiled array was created
# internally.
circuits_ref = qi.transpile(circuit) # always returns a new array
circuits_input = circuits_ref.copy()
_ = qi.execute(circuits_input, had_transpiled=True)
self.assertEqual(circuits_ref, circuits_input, msg="Transpiled circuit array modified.")


if __name__ == "__main__":
unittest.main()