simplify the noise model construction for noisy simulation with FakeBackendV2 #8979

cometta · 2022-10-21T06:37:36Z

Summary

Add back noisy simulation of Pulse job for FakeBackendV2: fixes Add back noisy simulation of Pulse job for FakeBackendV2 after Aer adds support for BackendV2 #7831
Update FakeBackendV2.run method after Aer adds support for BackendV2: fixes Update FakeBackendV2.run method after Aer adds support for BackendV2 #7829

Details and comments

qiskit-bot · 2022-10-21T06:37:40Z

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 the following people are requested to review this:

@Qiskit/terra-core

itoko

Thank you for working on this. I suggest to change the scope of the PR since Aer's pulse simulator does not support BackendV2. And we need to care about the users of qiskit_aer<0.11.0 since BackendV2 support of Aer's NoiseModel is added in 0.11.0 (very recent).

itoko · 2022-11-02T09:18:48Z

qiskit/providers/fake_provider/fake_backend.py

-        if pulse_job:  # pulse job
-            raise QiskitError("Pulse simulation is currently not supported for V2 fake backends.")


You need to keep these lines. To run pulse job, you need to do the same things as in FakeBackend(BackendV1).run. However, PulseSystemModel has not yet support BackendV2 so you cannot to have it. I suggest to change the scope of this PR and focus on just replacing the _get_noise_model_from_backend_v2 with from_backend.

Hello @itoko thank you, i have done the changes based on your advice, no more error in ci/cd

@HuangJunye can you review the code as well. let me know if further changes ?

t-imamichi · 2022-11-02T14:34:13Z

test/python/primitives/test_backend_sampler.py

@@ -193,8 +193,8 @@ def test_run_2qubit(self, backend):

        self.assertDictAlmostEqual(result.quasi_dists[0], {0: 1}, 0.1)
        self.assertDictAlmostEqual(result.quasi_dists[1], {1: 1}, 0.1)
-        self.assertDictAlmostEqual(result.quasi_dists[2], {2: 1}, 0.1)
-        self.assertDictAlmostEqual(result.quasi_dists[3], {3: 1}, 0.1)
+        self.assertDictAlmostEqual(result.quasi_dists[2], {2: 1}, 0.2)


Why do you need to change delta values?

@t-imamichi , i have to put 0.2, otherwise, ci/cd has error. Can you advice if this is not needed? @itoko, what's your thoughts on this?

This change implies that this PR amplifies the error of BackendSampler with a noise model.
Does this PR set a different noise parameter values?

this PR is to removed the function that generate fake noise _get_noise_model_from_backend_v2 and use real noise noise_model = NoiseModel.from_backend(self) from Aer. That's why the noise has different value. @itoko your comment

Strange.. I'll take a deeper look at where the increase of error comes from early next week.

I've looked into the above unexpected behavior and found two issues:

A bug in Aer where excessive errors (other than ReadoutError) are added to measure instructions only when the noises are generated from Target object (Edit: -> Fix device noise model from BackendV2 not to have excessive quantum errors on measures/resets qiskit-aer#1649)

An issue in convert_to_target in Terra, that drops asymmetric readout error properties (prob_meas0_prep1 and prob_meas1_prep0) defined in props_dict since they are not defined in InstructionProperties. We may need to accept the drops or introduce a new MeasureInstructionProperties to store them additionally. (Edit: -> Support asymmetric readout errors in FakeBackendV2 #9097)

I'll fix the first bug as soon as possible (hopefully the fix should be included in aer 0.11.2).
As for the second issue, I'll create an issue to discuss how to address it.
Sorry, please wait for them, @cometta

Thanks for looking into this, Itoko-san.

@cometta: This is why we have tests - when stuff starts failing unexpectedly, we need to figure out why it's failing, not modify the tests to mask it. Tests are more than just things that need to be made to pass, they're also a tool for us to help find errors in our code.

itoko · 2022-11-04T10:32:52Z

releasenotes/notes/add-back-noise-simulation-fake-backend-v2-1ec971ac47568e18.yaml

+features:
+  - |
+    Reverting to the previous behavior. 
+    Add back noisy simulation of Pulse job for FakeBackendV2. 


This line should be removed now. Also please update the title and top message of this PR as well.

@itoko i can remove the release note yaml file. Can advice on what is the suitable title for this PR ?

I think we don't need to remove the release note, rather we need to update the release note to tell aer >= 0.11.2 (previously, no version restriction for Aer) is now required for users who want noisy simulation with FakeBackendV2 (maybe in others or upgrades section).
Regarding title, I would say Simplify the noise model construction for noisy simulation with FakeBackendV2 or something but you could think of a better one.

i reverted back release note with extra note you mentioned on versioning. I also updated this PR title according to your recommendation

itoko · 2022-11-04T10:35:08Z

qiskit/providers/fake_provider/fake_backend.py

@@ -119,10 +119,15 @@ def _parse_channels(self, channels):
    def _setup_sim(self):
        if _optionals.HAS_AER:


Only aer > 0.11.0 supports BackendV2 so we may need to check the version of Aer here. And we may need to tell users to fall back to BasicAer in that case by changing the following warning message in run method.

if not _optionals.HAS_AER: warnings.warn("Aer not found using BasicAer and no noise", RuntimeWarning)

@itoko i implemented the version checking feature that you suggested and give warning if ae < 0.11.0

Thanks but I'm not sure the updated code works well with aer < 0.11.0. Have you tested it with aer < 0.11.0? I guess the same version check may be necessary in _setup_sim.
@HuangJunye With this PR, we are changing to require aer >= 0.11.2 for FackBackendV2 users who want to do noisy simulation. I personally don't think it's too restrictive so I'm fine with it but I'd like to hear what others think about it.

This is a new feature and Aer is just an optional accelerator/extender here, so it will be ok if it can't be made to work with Aer < 0.11 - this PR won't go out until at least Terra 0.23 anyway. However, it might not hurt to leave in the old function for a release, and continue to use it if Aer 0.10 (or whatever the minimum supported version is) is present.

With the inline version parsing here, I'm considering adding versioning methods to the lazy import checkers, which should simplify some of this. I've made #9086 to track that.

@jakelishman can the versioning methods to the lazy import checkers with helper functions consider as a separate ticket? For this ticket just hardcode checking of versioning? I rebase the code as terra just released 0.23 . Let me know what else can i do so that this ticket can be close asap

Yes, they would be a separate ticket (I made #9086 to track it, and I'll add the feature when I have time). Terra 0.23.0 hasn't been released yet - it's due in January. We released Terra 0.22.3 yesterday, which might have been the confusion.

i pushed the code to run for Terra 0.23.0 still has error in ci/cd . Can comment @jakelishman

jakelishman

Thanks for looking at this. I have a couple of concerns about things that are being done in the PR, but in general it'll be good for us to rely on Aer's native support again.

jakelishman · 2022-11-07T11:05:57Z

qiskit/providers/fake_provider/fake_backend.py

+
+        if _optionals.HAS_AER:
+            from pkg_resources import parse_version
+            from qiskit_aer import version as aer_version


This cannot succeed if Aer is <0.11, because it didn't publish itself with this name until Aer 0.11.0.

@jakelishman is the preferable way to get version is using qiskit.__qiskit_version__ to check for aer version?

jakelishman · 2022-11-07T11:10:36Z

qiskit/providers/fake_provider/fake_backend.py

+                with warnings.catch_warnings():
+                    warnings.filterwarnings(
+                        "ignore", category=UserWarning, module="qiskit_aer.noise.device"
+                    )


Why are we suppressing warnings? The warnings are something we should look to fix, not hide - either Aer is spuriously warning, or on the Terra side we need to first modify our data so that it doesn't cause warnings; it's the caller here that has the most information to decide how to modify the data to avoid the warnings. I suspect this is related to Aer issues about spuriously warning, in which case we should not put in the filter, and let Aer fix them.

Right. This is a consequence of a bug in Aer and I've just fixed it in Qiskit/qiskit-aer#1639 (merged yesterday). These lines should be removed. Sorry @cometta for not clearly telling this suppression is a temporary solution that should be tested in your local environment.

i removed above lines

jakelishman · 2022-11-07T11:11:42Z

qiskit/providers/fake_provider/fake_backend.py

-        if not _optionals.HAS_AER:
+
+        if _optionals.HAS_AER:
+            from pkg_resources import parse_version


Terra doesn't have a direct requirement on pkg_resources (and this module is effectively deprecated anyway), so we can't use this.

@jakelishman should i wait for the helper function #9086 to check versioning or use other python function to check for the version for now? pkg_resources comes together with python installation. there is no need to install additional python library

jakelishman · 2022-11-07T11:14:57Z

test/python/primitives/test_backend_sampler.py

@@ -193,8 +193,8 @@ def test_run_2qubit(self, backend):

        self.assertDictAlmostEqual(result.quasi_dists[0], {0: 1}, 0.1)
        self.assertDictAlmostEqual(result.quasi_dists[1], {1: 1}, 0.1)
-        self.assertDictAlmostEqual(result.quasi_dists[2], {2: 1}, 0.1)
-        self.assertDictAlmostEqual(result.quasi_dists[3], {3: 1}, 0.1)
+        self.assertDictAlmostEqual(result.quasi_dists[2], {2: 1}, 0.2)


Thanks for looking into this, Itoko-san.

@cometta: This is why we have tests - when stuff starts failing unexpectedly, we need to figure out why it's failing, not modify the tests to mask it. Tests are more than just things that need to be made to pass, they're also a tool for us to help find errors in our code.

jakelishman · 2022-11-07T11:15:23Z

test/python/providers/test_fake_backends.py

-    @combine(
-        backend=[be for be in FAKE_PROVIDER_FOR_BACKEND_V2.backends() if be.num_qubits > 1],
-        optimization_level=[0, 1, 2, 3],
-    )
-    def test_circuit_on_fake_backend_v2(self, backend, optimization_level):
-        if not optionals.HAS_AER and backend.num_qubits > 20:
-            self.skipTest("Unable to run fake_backend %s without qiskit-aer" % backend.backend_name)
-        job = execute(
-            self.circuit,
-            backend,
-            optimization_level=optimization_level,
-            seed_simulator=42,
-            seed_transpiler=42,
-        )
-        result = job.result()
-        counts = result.get_counts()
-        max_count = max(counts.items(), key=operator.itemgetter(1))[0]
-        self.assertEqual(max_count, "11")
-


Why are we removing this test?

i reverted it.

jakelishman · 2022-11-07T11:38:40Z

qiskit/providers/fake_provider/fake_backend.py

@@ -119,10 +119,15 @@ def _parse_channels(self, channels):
    def _setup_sim(self):
        if _optionals.HAS_AER:


This is a new feature and Aer is just an optional accelerator/extender here, so it will be ok if it can't be made to work with Aer < 0.11 - this PR won't go out until at least Terra 0.23 anyway. However, it might not hurt to leave in the old function for a release, and continue to use it if Aer 0.10 (or whatever the minimum supported version is) is present.

With the inline version parsing here, I'm considering adding versioning methods to the lazy import checkers, which should simplify some of this. I've made #9086 to track that.

cometta · 2022-11-08T05:53:05Z

@itoko @jakelishman are you able to advice on fail on unit-tests when running ci/cd. Is it due to aer ?

itoko · 2022-11-10T08:07:33Z

@cometta It seems you are getting two sets of test failures: One is for docs and the other is due to Aer. I guess the first one might be due to some invalid strings in release note. As for the second one, it is expected failures, let's wait for Aer's bug-fix.

coveralls · 2023-02-03T02:54:16Z

Pull Request Test Coverage Report for Build 4080494526

6 of 8 (75.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.0006%) to 85.245%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/providers/fake_provider/fake_backend.py	6	8	75.0%

Files with Coverage Reduction	New Missed Lines	%
qiskit/providers/fake_provider/fake_backend.py	1	90.63%

Totals
Change from base Build 4077166277:	0.0006%
Covered Lines:	67037
Relevant Lines:	78640

💛 - Coveralls

cometta · 2023-02-06T07:34:15Z

@jakelishman terra 0.23 already released, the ci/cd test still has error. can comment how to proceed?

-Update FakeBackendV2.run method

t-imamichi · 2023-02-08T06:10:05Z

I tried this PR locally and it fails at test.python.primitives.test_backend_sampler.TestBackendSampler.test_run_empty_parameter_2.

======================================================================
FAIL: test_run_empty_parameter_2 (test.python.primitives.test_backend_sampler.TestBackendSampler)
test.python.primitives.test_backend_sampler.TestBackendSampler.test_run_empty_parameter_2
----------------------------------------------------------------------
testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/Users/ima/envs/dev310/lib/python3.10/site-packages/ddt.py", line 220, in wrapper
    return func(self, *args, **kwargs)
  File "/Users/ima/tasks/3_2023/qiskit/terra/test/python/primitives/test_backend_sampler.py", line 238, in test_run_empty_parameter
    self.assertDictAlmostEqual(quasi_dist, {0: 1.0}, delta=0.1)
  File "/Users/ima/tasks/3_2023/qiskit/terra/qiskit/test/base.py", line 146, in assertDictAlmostEqual
    raise self.failureException(msg)
AssertionError: (0: 0.899 != 1.0) within 0.1 delta

The backend noise model seems different from one of the main branch. Do you have any idea, @itoko?

jakelishman · 2023-02-08T11:51:39Z

Yeah, that looks like a true test failure caused by this change, but it's sufficiently close that it's maybe that the test is too strict. Ideally we should do the maths and work out what the approximate margin of error is for that test with a given noise model, and either increase its number of shots (within reason) or relax the tightness of the test a bit.

t-imamichi · 2023-02-09T02:22:02Z

The quasi_dist value of this PR (in line 230 of test_backend_sampler.py) is

{3: 0.001, 6: 0.001, 4: 0.033, 1: 0.017, 0: 0.893, 2: 0.025, 12: 0.003, 8: 0.027}.

That of main branch is

{4: 0.01, 1: 0.011, 8: 0.018, 2: 0.013, 0: 0.948}.

The circuit is as follows.

        n = 5
        qc = QuantumCircuit(n, n - 1)
        qc.measure(range(n - 1), range(n - 1))

The difference looks larger than sampling error. I'm not sure what causes it.

jakelishman · 2023-02-09T11:11:15Z

We can't make that call without knowing the size of the measurement error in each branch (since this PR looks at noise models), the number of shots, and any error mitigation that's being done, right?

t-imamichi · 2023-02-09T12:33:57Z

The number of shots is 1000 and no mitigation is applied in the test.

cometta requested review from a team and jyu00 as code owners October 21, 2022 06:37

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Oct 21, 2022

cometta mentioned this pull request Oct 21, 2022

Add back noisy simulation of Pulse job for FakeBackendV2 after Aer adds support for BackendV2 #7831

Open

cometta force-pushed the issue-#7831 branch from 05f1451 to 1a9f653 Compare November 1, 2022 08:37

cometta requested review from ikkoham and t-imamichi as code owners November 1, 2022 12:55

itoko requested changes Nov 2, 2022

View reviewed changes

t-imamichi reviewed Nov 2, 2022

View reviewed changes

cometta force-pushed the issue-#7831 branch from 0d6fbaf to 07f565b Compare November 3, 2022 05:46

itoko reviewed Nov 4, 2022

View reviewed changes

jakelishman mentioned this pull request Nov 7, 2022

Add a method to specify minimum versions in the lazy optional checkers #9086

Open

jakelishman reviewed Nov 7, 2022

View reviewed changes

cometta force-pushed the issue-#7831 branch from 6b455c0 to 2c8b976 Compare November 8, 2022 05:10

cometta changed the title ~~-Add back noisy simulation of Pulse job for FakeBackendV2~~ simplify the noise model construction for noisy simulation with FakeBackendV2 Nov 8, 2022

itoko mentioned this pull request Nov 8, 2022

Support asymmetric readout errors in FakeBackendV2 #9097

Open

cometta force-pushed the issue-#7831 branch from be17d14 to 419d507 Compare November 24, 2022 01:08

cometta force-pushed the issue-#7831 branch from 419d507 to 75f0669 Compare February 3, 2023 02:23

cometta added 6 commits February 8, 2023 13:52

-Add back noisy simulation of Pulse job for FakeBackendV2

09867d7

-Update FakeBackendV2.run method

added release note

952c755

removed un-used testcase

e8d8327

fix unitest after revert backendV2

537c1f1

fix ci/cd error

7f90190

change delta

ec376c1

cometta added 5 commits February 8, 2023 13:52

retrigger

f059feb

retrigger

bd8fdf2

check Aer > 0.11.0 otherwise show warning

1320de6

revert unittests

98b2cfd

revert release note

dbc6aac

cometta force-pushed the issue-#7831 branch from 75f0669 to dbc6aac Compare February 8, 2023 05:52

javabster requested a review from jakelishman May 23, 2023 16:09

		if pulse_job: # pulse job
		raise QiskitError("Pulse simulation is currently not supported for V2 fake backends.")

		@@ -119,10 +119,15 @@ def _parse_channels(self, channels):
		def _setup_sim(self):
		if _optionals.HAS_AER:

simplify the noise model construction for noisy simulation with FakeBackendV2 #8979

Are you sure you want to change the base?

simplify the noise model construction for noisy simulation with FakeBackendV2 #8979

Conversation

cometta commented Oct 21, 2022 • edited by javabster Loading

Summary

Details and comments

qiskit-bot commented Oct 21, 2022

itoko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cometta Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itoko Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itoko Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itoko Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

cometta Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

itoko Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cometta Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cometta Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cometta commented Nov 8, 2022

itoko commented Nov 10, 2022

coveralls commented Feb 3, 2023

Pull Request Test Coverage Report for Build 4080494526

💛 - Coveralls

cometta commented Feb 6, 2023

t-imamichi commented Feb 8, 2023

jakelishman commented Feb 8, 2023

t-imamichi commented Feb 9, 2023

jakelishman commented Feb 9, 2023

t-imamichi commented Feb 9, 2023

cometta commented Oct 21, 2022 •

edited by javabster

Loading

cometta Nov 4, 2022 •

edited

Loading

itoko Nov 7, 2022 •

edited

Loading

itoko Nov 4, 2022 •

edited

Loading

itoko Nov 7, 2022 •

edited

Loading

cometta Nov 8, 2022 •

edited

Loading

itoko Nov 4, 2022 •

edited

Loading

cometta Nov 8, 2022 •

edited

Loading

cometta Nov 8, 2022 •

edited

Loading