Update calibration and mixin tests to work with BackendV2

[wshanks]

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 than BackendV1. Additionally it adds support for querying BackendV2 for measurement frequencies.

Details and comments

The main changes are:

• Use backend.defaults() to look up measure_freq_est for BackendV2 as well as BackendV1.
• Improve the error message when measure_freq_est can not be accessed for ResonatorSpectroscopy
• 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 are a few of inelegant changes in this PR:

• Usage of backend.defaults() was restored for accessing the measurement frequencies. This is an IBM specific method not in the BackendV1 or BackendV2 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 use defaults() 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 fake BackendV2 backends for the ResonatorSpectroscopy tests. The fake BackendV1 backends in terra do provide it but not the BackendV2 variants. One nice thing about moving away from defaults() (previous point) is that we could remove this hack.
• There is no BackendV2 version of FakeOpenPulse2Q, so one was hacked out of the BackendV1 version. Perhaps we could get terra to include a BackendV2 version of FakeOpenPulse2Q in the future.

Closes #1099

[nkanazawa1989]

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.

[nkanazawa1989]

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

[wshanks]

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.

[nkanazawa1989]

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().

[nkanazawa1989]

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.

[wshanks]

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.

[wshanks]

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.

[nkanazawa1989]

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.

[nkanazawa1989]

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.

[wshanks]

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).

[nkanazawa1989]

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.

[nkanazawa1989]

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.

https://github.com/Qiskit/qiskit-ibm-provider/blob/06490ab5ccfabc1ceae79bc3e8ba3f8bb82296fa/qiskit_ibm_provider/ibm_backend.py#L714-L735

[wshanks]

@nkanazawa1989 Could you review this again? I have scaled back the use of Backend.defaults() to only the necessary cases (ResonatorSpectroscopy), so I think there is less to debate in this PR. To address that case, I think the discussion and work should proceed from the issue you opened (Qiskit/qiskit-ibm-provider#387) so we don't need to do more here.

The one remaining ugly part of this PR is the way I define FakeOpenPulse2QV2. Ideally, terra would provide a V2 version of FakeOpenPulse2Q. We started discussing that back in Qiskit/qiskit#8712. I would say that that discussion can proceed there and we can live with this hacked version for now.

Note that this PR should address the problem described in #1099.

[wshanks]

I updated the PR description to match the current status of the PR.

[wshanks]

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

[nkanazawa1989]

Thanks Will. This change looks almost good to me. Just some minor questions.

[nkanazawa1989]

I'm curious about the reason not to use BackendV2Converter.

[wshanks]

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.

[wshanks]

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:

https://github.com/Qiskit/qiskit-terra/blob/a1a67a11741000f8f3b6d6a8766dab7e3cd06847/qiskit/providers/backend_compat.py#L86-L89

while FakeBackendV2 checks for readout_length but does not error if it is not there:

https://github.com/Qiskit/qiskit-terra/blob/a1a67a11741000f8f3b6d6a8766dab7e3cd06847/qiskit/providers/fake_provider/utils/backend_converter.py#L80-L85

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.

[nkanazawa1989]

Thanks Will. This is good to know. I agree we should have Fake V2 in Terra in future.

[wshanks]

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.

[nkanazawa1989]

Thanks Will. This looks good to me.