-
Notifications
You must be signed in to change notification settings - Fork 126
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.
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
Update calibration and mixin tests to work with BackendV2 #900
Changes from 3 commits
25fadf2
e4bf2a8
b4312be
c8ab136
f386e78
9923e92
babf0ec
8847ac1
57336cc
2d8a247
02dc7eb
a9d0c38
44cd94d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,29 +205,6 @@ def provider(self): | |
return None | ||
return None | ||
|
||
@property | ||
def drive_freqs(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we don't need to delete these methods. Probably we can update API so that this can take index of target element -- like t1 method you implemented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing that confuses me is the role of I don't think it is worth giving up access to the measurement frequencies when using V2 because the measurement frequencies are still available in the same place we have been referencing for V1. Moving away from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One reason my last comment is pedantic -- while in calibrations we are interested in determining the frequencies at which to drive the control and measurement tones, all of the usages of drive_freqs/meas_freqs currently are rough numbers -- used either to set default frequencies for calibrations and center frequencies for spectroscopy. The resonant frequencies are probably good enough as starting points. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is good point. We also need to think about how dynamics simulator consumes this backend to build a device Hamiltonian. Thus clear separation of device parameters from the control parameters is reasonable way to go. I'm also fine with doing this in two steps as you suggest, namely re-introducing the defaults in IBM V2 backends. What I really don't like is the separation of device vs control parameter is not clear in these objects. I think we need the refactoring of data structure with V2. (edit) I think |
||
"""Returns the backend's qubit drive frequencies""" | ||
if self._v1: | ||
return getattr(self._backend.defaults(), "qubit_freq_est", []) | ||
elif self._v2: | ||
return [property.frequency for property in self._backend.target.qubit_properties] | ||
return [] | ||
|
||
@property | ||
def meas_freqs(self): | ||
"""Returns the backend's measurement stimulus frequencies. | ||
|
||
.. note:: | ||
Currently BackendV2 does not have access to this data. | ||
""" | ||
if self._v1: | ||
return getattr(self._backend.defaults(), "meas_freq_est", []) | ||
elif self._v2: | ||
# meas_freq_est is currently not part of the BackendV2 | ||
return [] | ||
return [] | ||
|
||
@property | ||
def num_qubits(self): | ||
"""Returns the backend's number of qubits""" | ||
|
@@ -255,3 +232,18 @@ def is_simulator(self): | |
return True | ||
|
||
return False | ||
|
||
def qubit_t1(self, qubit: int) -> float: | ||
"""Return the T1 value for a qubit from the backend properties | ||
|
||
Args: | ||
qubit: the qubit index to return T1 for | ||
|
||
Returns: | ||
The T1 value | ||
""" | ||
if self._v1: | ||
return self._backend.properties().qubit_property(qubit)["T1"][0] | ||
if self._v2: | ||
return self._backend.qubit_properties(0).t1 | ||
wshanks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return float("nan") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
# This code is part of Qiskit. | ||
# | ||
# (C) Copyright IBM 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 | ||
# of this source tree or at http://www.apache.org/licenses/LICENSE-2.0. | ||
# | ||
# Any modifications or derivative works of this code must retain this | ||
# copyright notice, and modified files need to carry a notice indicating | ||
# that they have been altered from the originals. | ||
|
||
"""Fake backend class for tests.""" | ||
|
||
from qiskit.exceptions import QiskitError | ||
from qiskit.providers.fake_provider import FakeArmonkV2, FakeAthensV2, FakeBelemV2 | ||
from qiskit.providers.models import PulseDefaults | ||
from qiskit.providers.fake_provider.utils.json_decoder import decode_pulse_defaults | ||
|
||
|
||
class PulseDefaultsMixin: | ||
"""Mixin class to add defaults() to a fake backend | ||
|
||
In particular, this class works with | ||
``qiskit.providers.fake_provider.fake_backend.FakeBackendV2`` classes that | ||
have a ``defs_filename`` attribute with the path to a pulse defaults json | ||
file. | ||
""" | ||
|
||
_defaults = None | ||
|
||
def defaults(self): | ||
"""Returns a snapshot of device defaults""" | ||
if not self._defaults: | ||
self._set_defaults_from_json() | ||
return self._defaults | ||
|
||
def _set_defaults_from_json(self): | ||
if not self.props_filename: | ||
raise QiskitError("No properties file has been defined") | ||
defs = self._load_json(self.defs_filename) | ||
decode_pulse_defaults(defs) | ||
self._defaults = PulseDefaults.from_dict(defs) | ||
|
||
|
||
class FakeArmonkV2Pulse(FakeArmonkV2, PulseDefaultsMixin): | ||
"""FakeArmonkV2 with pulse defaults""" | ||
|
||
|
||
class FakeAthensV2Pulse(FakeAthensV2, PulseDefaultsMixin): | ||
"""FakeAthensV2 with pulse defaults""" | ||
|
||
|
||
class FakeBelemV2Pulse(FakeBelemV2, PulseDefaultsMixin): | ||
"""FakeBelemV2 with pulse defaults""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,10 @@ | |
import numpy as np | ||
|
||
from qiskit import QuantumCircuit | ||
from qiskit.circuit.library import XGate, SXGate | ||
from qiskit.result import Result | ||
from qiskit.providers.fake_provider import FakeOpenPulse2Q | ||
from qiskit.providers.fake_provider.fake_backend import FakeBackendV2 | ||
|
||
from qiskit.qobj.utils import MeasLevel | ||
from qiskit_experiments.exceptions import QiskitError | ||
|
@@ -31,7 +33,46 @@ | |
) | ||
|
||
|
||
class MockRestlessBackend(FakeOpenPulse2Q): | ||
class FakeOpenPulse2QV2(FakeBackendV2): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious about the reason not to use BackendV2Converter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main reason is that I started this PR before it existed. I wonder if it would work or would suffer from the issues that I hack around below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried this: from qiskit.providers.fake_provider import FakeOpenPulse2Q
from qiskit.providers.backend_compat import BackendV2Converter
class FakeOpenPulse2QV2(BackendV2Converter):
def __init__(self):
v1backend = FakeOpenPulse2Q()
super().__init__(v1backend) but I encountered several test errors. A lot of them were this problem:
while I tried working around that with this (thisi is also working around errors from from qiskit.providers.fake_provider import FakeOpenPulse2Q
from qiskit.providers.backend_compat import BackendV2Converter
class FakeOpenPulse2QV2(BackendV2Converter):
def __init__(self):
backend_v1 = FakeOpenPulse2Q()
backend_v1.configuration().description = "FakeOpenPulse2Q"
backend_v1.configuration().max_experiments = 100
cmd_def = backend_v1.defaults().cmd_def
measure = next(c for c in cmd_def if c.name == "measure")
readout_length = max(getattr(i, "duration", 0) for i in measure.sequence)
for qubit in backend_v1._properties._qubits.values():
qubit["readout_length"] = (readout_length * backend_v1.configuration().dt, qubit["readout_error"][1])
super().__init__(backend_v1, add_delay=True)
self._defaults = backend_v1._defaults but I still had further errors. The next one that needed to be addressed was that At that point, I decided it was more trouble than it was worth to get this working. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks Will. This is good to know. I agree we should have Fake V2 in Terra in future. |
||
"""BackendV2 version of FakeOpenPulse2Q""" | ||
|
||
def __init__(self): | ||
# FakeOpenPulse2Q has its data hard-coded rather than in json files. We | ||
# prepopulate the dicts so that FakeBackendV2 does not try to load them | ||
# from files. | ||
# | ||
# We have to use a hack to populate _conf_dict | ||
backend_v1 = FakeOpenPulse2Q() | ||
self._conf_dict_real = backend_v1._configuration.to_dict() | ||
super().__init__() | ||
self._props_dict = backend_v1._properties.to_dict() | ||
self._defs_dict = backend_v1._defaults.to_dict() | ||
self._defaults = backend_v1._defaults | ||
|
||
# Workaround a bug in FakeOpenPulse2Q. It defines u1 on qubit 1 in the | ||
# cmd_def in the defaults json file but not in the gates in the | ||
wshanks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# properties json. The code FakeBackendV2 uses to build the Target | ||
wshanks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# assumes these two are consistent. | ||
u1_0 = next(g for g in self._props_dict["gates"] if g["gate"] == "u1") | ||
self._props_dict["gates"].append(u1_0.copy()) | ||
self._props_dict["gates"][-1]["qubits"] = [1] | ||
|
||
@property | ||
def _conf_dict(self): | ||
# FakeBackendV2 sets this in __init__. As a hack, we use this property | ||
# to prevent it from overriding our values. | ||
return self._conf_dict_real | ||
|
||
@_conf_dict.setter | ||
def _conf_dict(self, value): | ||
pass | ||
|
||
def defaults(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this method is not provided by the provider, I'm not sure if this is really right approach. Actually this mixin generates the mixture of V1 and V2 model, which will never exist outside the QE -- so I'm not sure what is tested by this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think we could probably make a PR to terra to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I think updating terra code (probably provider too) is proper direction. Otherwise, we cannot get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry I didn't notice IBM V2 backend reports defaults object. So what you are doing seems right. We just need to update FakeV2 backends based on IBM one in terra. |
||
"""Pulse defaults""" | ||
return self._defaults | ||
|
||
|
||
class MockRestlessBackend(FakeOpenPulse2QV2): | ||
"""An abstract backend for testing that can mock restless data.""" | ||
|
||
def __init__(self, rng_seed: int = 0): | ||
|
@@ -144,7 +185,8 @@ def __init__( | |
self._angle_per_gate = angle_per_gate | ||
super().__init__(rng_seed=rng_seed) | ||
|
||
self.configuration().basis_gates.extend(["sx", "x"]) | ||
self.target.add_instruction(SXGate(), properties={(0,): None}) | ||
self.target.add_instruction(XGate(), properties={(0,): None}) | ||
|
||
def _compute_outcome_probabilities(self, circuits: List[QuantumCircuit]): | ||
"""Compute the probabilities of being in the excited state or | ||
|
@@ -170,7 +212,7 @@ def _compute_outcome_probabilities(self, circuits: List[QuantumCircuit]): | |
self._precomputed_probabilities[(idx, "01")] = [prob_1, prob_0, 0, 0] | ||
|
||
|
||
class MockIQBackend(FakeOpenPulse2Q): | ||
class MockIQBackend(FakeOpenPulse2QV2): | ||
"""A mock backend for testing with IQ data.""" | ||
|
||
def __init__( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add TODO comment so that not to forget? I think we should upgrade "IBM" V2 model properly. When we designed the
QubitProperty
class, we tried to avoid adding superconducting technology specific properties, so that terra is technology agnostic. We expected IBM provider to override this class to return cavity frequency and anharmonicity, and actually it returnsIBMQubitProperty
. However, cavity frequency is currently missing. And I'm not sure if this is the right place to report readout frequency. I wrote an issue in the provider and hope you will add some comment to Qiskit/qiskit-ibm-provider#387There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the TODO for? Removing usage of
backend.defaults()
? Maybe we should revisit this after more discussion of your next comment aboutdrive_freqs
and Qiskit/qiskit-ibm-provider#387.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
.congirutation()
and.properties()
inBackendV2
are something temporary for migration and will be deprecated gradually, @mtreinish ? So I don't want to permanently introduce.defaults()
.