-
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
Conversation
* Restore direct `backend.defaults()` access. `backend.defaults` is IBM-specific but is not BackendV1/BackendV2 specific, so it does not need to be abstracted in `BackendData`. For some tests, this required adding `.defaults` to the BackendV2 backend they were using because the BackendV2 backends in terra do not provide it. * Add T1 to `BackendData` because it is needed by `RestlessMixin`. * Switch `MockRestlessBackend` and `MockIQBackend` to BackendV2. This conversion was done by making a `FakeOpenPulse2QV2` BackendV2 shim for the BackendV1 `FakeOpenPulse2Q` in terra. * Replace modification of `basis_gates` with modification of `backend.target` in relevant tests. * Remove some unnecessary code (assignments to `timing_constraints` and a test class with no tests).
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.
Thanks Will. Seems like this is the great start of V2 migration of calibrations, we intentionally ignored in #843.
However, I'm not sure if creating virtual instance augmented by defaults object is really right approach for testing, because such object will be never provided. I'm fine if this is temporary workaround until readout frequency is provided and upgrade of these tests are urgent. Otherwise I prefer to upgrade provider and accordingly fake backends in terra.
Regarding the test during migration period, perhaps we can write couple of integration tests, and run data driven test with V1 and V2 objects.
@@ -275,10 +275,10 @@ def from_backend( | |||
) | |||
|
|||
if add_parameter_defaults: | |||
for qubit, freq in enumerate(backend_data.drive_freqs): | |||
for qubit, freq in enumerate(getattr(backend.defaults(), "qubit_freq_est", [])): |
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 returns IBMQubitProperty
. 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#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.
What is the TODO for? Removing usage of backend.defaults()
? Maybe we should revisit this after more discussion of your next comment about drive_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()
in BackendV2
are something temporary for migration and will be deprecated gradually, @mtreinish ? So I don't want to permanently introduce .defaults()
.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to delete these methods. meas_freqs
has note about missing frequency, and I think this is sufficient to communicate with users. Actually I don't like V1 model, because we have qubit frequencies in three places, i.e. backend.defaults().qubit_freq_est
, backend.properties().qubit_properties()
, and backend.configuration().hamiltonian
. This method unifies where to get frequency, and good for novice developers.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that confuses me is the role of PulseDefaults
now. I opened #877 about this. BackendData
provides compatibility between V1 and V2, but defaults()
is not in the base class for either V1 or V2. It is in the IBM classes for both V1 and V2. I think the IBM provider is the only one that implements qiskit pulse support, so it is hard to distinguish between what is IBM-specific and what is pulse-specific. I think it is fair for experiments referencing the drive frequencies to use pulse-specific API's (and so probably fair to use defaults()
unless all of its utility is supposed to be moved to Target
).
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 defaults()
seems fine if that is the direction that we are supposed to go. It might be better to do that in a separate step from initial V2 compatibility. This is a little pedantic but I think the frequencies should live in two places. One place (like backend.properties()
) can hold resonant frequencies of the device and the other (like backend.defaults()
or maybe backend.target
in the future) can hold frequencies used by the control electronics. For IBM superconducting transmon qubits, readout is done by coupling to a resonator whose frequency changes (by 2 chi) depending on the state of the qubit. Typically readout is done by driving half-way in between these two frequencies. There is less distinction for qubits but even there for finite ZZ one needs to decide to drive at the frequency of the qubit with its neighbors in 0 or with them in 0+1.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
One place (like backend.properties()) can hold resonant frequencies of the device and the other (like backend.defaults() or maybe backend.target in the future) can hold frequencies used by the control electronics.
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 BackendData
is the place where we can introduce this context to some extent, without future breaking API change.
def _conf_dict(self, value): | ||
pass | ||
|
||
def defaults(self): |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
defaults()
is not in the BackendV1
or BackendV2
base classes but it is the BackendV1
and BackendV2
subclasses produced by qiskit-ibmq-provider
and qiskit-ibm-provider
. The main thing that changed is that fake BackendV1
classes in terra implement defaults()
while the BackendV2
versions do not, so switching the tests to the V2 versions required addressing the defaults()
issue. The classes hold all the PulseDefaults
data. They just don't expose it through defaults()
. This is why there is so little code in fake_pulse_backends.py
.
I think we could probably make a PR to terra to add FakeOpenPulse2QV2
directly and remove the need for the hacks here (other than adding defaults()
back like fake_pulse_backends.py
).
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 we could probably make a PR to terra to add FakeOpenPulse2QV2 directly and remove the need for the hacks here (other than adding defaults() back like fake_pulse_backends.py).
Yes, I think updating terra code (probably provider too) is proper direction. Otherwise, we cannot get defaults
data set once we switch to actual V2 backends.
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.
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.
Co-authored-by: Naoki Kanazawa <[email protected]>
@nkanazawa1989 Could you review this again? I have scaled back the use of The one remaining ugly part of this PR is the way I define Note that this PR should address the problem described in #1099. |
I updated the PR description to match the current status of the PR. |
I should add a release note for the meas_freq change since it is now a bug fix. I think the rest of the changes are internal. Edit: Done in 02dc7eb |
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.
Thanks Will. This change looks almost good to me. Just some minor questions.
@@ -31,7 +33,49 @@ | |||
) | |||
|
|||
|
|||
class MockRestlessBackend(FakeOpenPulse2Q): | |||
class FakeOpenPulse2QV2(FakeBackendV2): |
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'm curious about the reason not to use BackendV2Converter.
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.
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 comment
The 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:
File "/home/wshanks/Documents/Code/qiskit-experiments/test/library/characterization/test_half_angle.py", line 39, in test_end_to_end
exp_data = hac.run(backend)
File "/home/wshanks/Documents/Code/qiskit-experiments/qiskit_experiments/framework/base_experiment.py", line 238, in run
transpiled_circuits = experiment._transpiled_circuits()
File "/home/wshanks/Documents/Code/qiskit-experiments/qiskit_experiments/framework/base_experiment.py", line 319, in _transpiled_circuits
transpiled = transpile(self.circuits(), self.backend, **transpile_opts)
File "/home/wshanks/.conda/envs/qiskit/lib/python3.10/site-packages/qiskit/compiler/transpiler.py", line 326, in transpile
unique_transpile_args, shared_args = _parse_transpile_args(
File "/home/wshanks/.conda/envs/qiskit/lib/python3.10/site-packages/qiskit/compiler/transpiler.py", line 656, in _parse_transpile_args
inst_map = _parse_inst_map(inst_map, backend)
File "/home/wshanks/.conda/envs/qiskit/lib/python3.10/site-packages/qiskit/compiler/transpiler.py", line 818, in _parse_inst_map
inst_map = backend.target.instruction_schedule_map()
File "/home/wshanks/.conda/envs/qiskit/lib/python3.10/site-packages/qiskit/providers/backend_compat.py", line 244, in target
self._target = convert_to_target(
File "/home/wshanks/.conda/envs/qiskit/lib/python3.10/site-packages/qiskit/providers/backend_compat.py", line 87, in convert_to_target
duration=properties.readout_length(qubit),
File "/home/wshanks/.conda/envs/qiskit/lib/python3.10/site-packages/qiskit/providers/models/backendproperties.py", line 453, in readout_length
return self.qubit_property(qubit, "readout_length")[0] # Throw away datetime at index 1
File "/home/wshanks/.conda/envs/qiskit/lib/python3.10/site-packages/qiskit/providers/models/backendproperties.py", line 389, in qubit_property
raise BackendPropertyError(
qiskit.providers.exceptions.BackendPropertyError: "Couldn't find the property 'readout_length' for qubit 0."
FakeBackendV2
follows a different code path from BackendConverter
. They use different convert_to_target
functions. BackendConverter
assumes that readout_length
is in the properties here:
while FakeBackendV2
checks for readout_length
but does not error if it is not there:
I tried working around that with this (thisi is also working around errors from description
and max_experiments
being missing):
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 u2
and id
were in the basis gates but not in the properties which led to an error in a transpiler pass.
At that point, I decided it was more trouble than it was worth to get this working. Maybe BackendConverter
should be more robust, but likely FakeOpenPulse2Q
just does not have complete enough metadata to match what is expected of a BackendV1 instance. We could fix that but likely it would be better just to make a BackendV2 equivalent class directly in terra.
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.
Thanks Will. This is good to know. I agree we should have Fake V2 in Terra in future.
@@ -31,7 +33,49 @@ | |||
) | |||
|
|||
|
|||
class MockRestlessBackend(FakeOpenPulse2Q): | |||
class FakeOpenPulse2QV2(FakeBackendV2): |
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.
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.
### Summary This change set updates the `FakeBackend` used by some tests to derive from `BackendV2` rather than `BackendV1` ### Details and comments When combined with [#900](#900) which updates some calibration tests and [#1040](#1040) which updates `T2HahnBackend`, this change results in almost all the tests in the test suite using `BackendV2`. The remaining tests using a `Backend` that is not `BackendV2` use the `AerSimulator` which will be updated to `BackendV2` in the future (tracked in Qiskit/qiskit-aer#1681). The check for backend type was performed by merging the calibration and `T2HahnBackend` branches into this branch and running the tests with `BaseExperiment.run()`, `BaseExperiment._set_backend()`, and `Calibrations.from_backend()` updated to error if the `backend` argument was not `BackendV2` or `AerSimulator` and confirming that all the tests passed.
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.
Thanks Will. This looks good to me.
Summary
This change continues the work started in #843 to make qiskit-experiments compatible with BackendV2. It updates the spectrscopy, fine frequency and amplitude, and T1 tests to use
BackendV2
rather thanBackendV1
. Additionally it adds support for queryingBackendV2
for measurement frequencies.Details and comments
The main changes are:
backend.defaults()
to look upmeasure_freq_est
forBackendV2
as well asBackendV1
.measure_freq_est
can not be accessed forResonatorSpectroscopy
BackendData
because it is needed byRestlessMixin
.MockRestlessBackend
andMockIQBackend
toBackendV2
. Thisconversion was done by making a
FakeOpenPulse2QV2
BackendV2
shim forthe
BackendV1
FakeOpenPulse2Q
in terra.basis_gates
with modification ofbackend.target
in relevant tests.timing_constraints
anda test class with no tests).
There are a few of inelegant changes in this PR:
backend.defaults()
was restored for accessing the measurement frequencies. This is an IBM specific method not in theBackendV1
orBackendV2
base classes. It is only used for querying meas_freq_est.backend.defaults()
is what we had been using before Backend v2 basic compatibility #843, so this is just putting things back the way we were. We might decide to get this data a different way that does not usedefaults()
but at the moment that is the only way to get the measurement frequencies, so we would need to the IBM backends to provide these values in the properties, for instance.defaults()
was hacked into the fakeBackendV2
backends for theResonatorSpectroscopy
tests. The fakeBackendV1
backends in terra do provide it but not theBackendV2
variants. One nice thing about moving away fromdefaults()
(previous point) is that we could remove this hack.BackendV2
version ofFakeOpenPulse2Q
, so one was hacked out of theBackendV1
version. Perhaps we could get terra to include aBackendV2
version ofFakeOpenPulse2Q
in the future.Closes #1099