Improve calibration experiments

[eggerdj]

Summary

This PR introduces a calibration base class that ties the update method to the experiment. The calibration experiments are updated accordingly.

Details and comments

Calibration experiments now subclass BaseCalibrationExperiment and they must implement the update_calibrations method which updates instances of Calibrations. This update will only run if an instance of Calibrations has been specified.

TODO

• Write tests
• Spectroscopy

[nkanazawa1989]

This is nice update. We can easily manage calibration, and user can write experiment in single line.

[wshanks]

I like putting the update ability into the experiment class. I put some comments on the BaseCalibrationExperiment class that I would be interested to hear @chriseclectic's opinions on regarding the intended usage of the BaseExperiment class.

[wshanks]

Should the Frequency.update() call be changed to use the class's update feature?

[wshanks]

Maybe the import of Amplitude should move here since it is not used in a code cell?

[wshanks]

It feels odd to me that a characterization experiment is a BaseCalibrationExperiment child. I wonder about making a RoughFrequency subclass of QubitSpectroscopy that is a BaseCalibrationExperiment. More generally, I wonder about making BaseCalibrationExperiment a mix-in class that can add on the updating feature to a BaseExperiment. For example, I like to use Rabi as a diagnostic experiment on a qubit, so I could also see Rabi as a characterization experiment with RoughAmplitude as a BaseCalibrationExperiment subclass.

On the other, I prefer to keep the class hierarchy as minimal as possible because long chains get confusing.

[eggerdj]

This is one of my main concerns. I have tried to make the calibration logic optional. I.e. we can still run QubitSpectroscopy independently of any calibration with the current PR. However, it does feel a bit funny to see calibration logic in the characterization code. I see the point about making BaseCalibrationExperiment a mix-in class but I also worry about making the class structure too complex to avoid confusion.

[nkanazawa1989]

I think

class RoughFrequency(QubitSpectroscopy, BaseCalibrationExperiment):
   ...

is a good suggestion though this could be confusing to users (especially finding the right experiment class).

[eggerdj]

I explore this in bb8fc79. It's not as bad as I though but you need to think carefully about the init order as we are dealing with a diamond inheritance structure here.

[nkanazawa1989]

I think

class RoughFrequency(QubitSpectroscopy, BaseCalibrationExperiment):
   ...

is a good suggestion though this could be confusing to users (especially finding the right experiment class).

[eggerdj]

With

class RoughFrequency(QubitSpectroscopy, BaseCalibrationExperiment):
   ...

we run the risk of having a class structure which is too confusing.

[nkanazawa1989]

Agreed, but this means QubitSpectroscopy can change the behavior with/without calibrations object. i.e. If calibrations is provided this runs parameter update, but without it the experiment just runs characterization.

This can cause some edge case. For example, how can we run experiment by using the calibrated pulses (this is not the case of qubit spect, but, for example, ef spect with calibrated X gate), but don't want to update the parameters? Perhaps this situation can be avoided by not providing cal object, but providing the X gate schedule via an exported backend with Qiskit/qiskit#6759

I prefer having two separate experiments that don't change its behavior depending input arguments.

[eggerdj]

In response to #251 (comment). As it stands, the calibration base class has an option auto_update which by default is set to true. If you don't want to update you just set that to false. No need for complex class mechanics to accommodate this case.

[wshanks]

This is looking good to me. It would be good to hear from @chriseclectic. To me, the outstanding questions with this PR are judgment calls about where to put complexity and flexibility -- whether it is better to have simple characterization experiments like Rabi and then child calibration experiments like RoughDrag, or whether Rabi should have rough amplitude calibration logic built into it.

[nkanazawa1989]

Thank Daniel. Calibration schedule construction is well standardized. I welcome the basic concept of this framework (now each experiment subclass implements the schedule construction logic in own style, and this is another confusing part for contributors).

On the other hand, calibration experiment code becomes pretty lengthy. I lean toward reducing the code at expense of generality. I guess the mix-in class approach can split the logic for circuit generation and parameter update, and it might make the class more readable.

[nkanazawa1989]

This is also fine but why we cannot call set_calibration_options? Seems like this bypasses some validations in the dedicated setter method.

[eggerdj]

What do you mean? Perhaps I misunderstand but we have a method def set_calibration_options(self, **fields): in BaseCalibrationExperiment.

[nkanazawa1989]

Yes, but this directly override the configuration in the constructor. We can always set an option via the setter method if it is not a configuration for the default value.

[chriseclectic]

As mentioned in above comment, it seems like this should be a fixed init kwarg, not an option that can be changed later?

[wshanks]

This looks good to me now. I think it is an improvement over how calibration updates are handled currently.

There are still some outstanding questions about the timing of the calibration update (if we don't want to block for results) but that requires more thought and work. Also, I think ideally we would find a way to create a calibration experiment by composing a characterization experiment with some options and an update method without the need for multiple inheritance. If we do change the design later, I hope it can be done without too much disruption to calibrations implemented in the time in between.

[nkanazawa1989]

This looks okey, but one minor comment. The BaseCalibrationExperiment implements _get_schedule_from_options and _get_schedule_from_defaults method, but these are usually already implemented in the characterization experiment which is a part of subclass of the calibration experiment.

For example, if we look at spectroscopy experiment, characterization one has this method. This is what _get_schedule_from_defaults provides. Seems like base calibration only need to provide a logic to get schedule from calibration.
https://github.com/Qiskit/qiskit-experiments/blob/22b1598b290464e59a12853efa1eb99b1b36513a/qiskit_experiments/library/characterization/qubit_spectroscopy.py#L133-L151

I hope this will be addressed in the following PRs.

[eggerdj]

Replying to: #251 (review) Yes, once we have terra 0.19 we should switch to a methodology that uses an instruction schedule map in combination with the transpil options.

[chriseclectic]

This looks good. I have a couple of small questions and suggestions. The main one is concerning the updater: it seems like it would be better to be passed as an init kwarg for the base class like the other base class params rather than being a class attribute.

[chriseclectic]

What is mro?

[wshanks]

mro stands for "method resolution order". This gives the order in which an instance's parent classes are checked for a method or attribute.

The usage pattern for BaseCalibrationExperiment here is to subclass both BaseCalibrationExperiment and a characterization experiment. BaseCalibrationExperiment's __init__ needs to be called first so it can do its setup and then call super().__init__ to run the characterization experiment's setup. For this to happen, the calibration experiment subclass needs to inherit from BaseCalibrationExperiment before the characterization experiment. If you do it the other way, you will probably figure out that things are not working right and fix your class definition, but this check is just a helper to the developer since it is an unusual pattern.

It would be nice to avoid asking calibration developers to use multiple inheritance. I think the multiple inheritance method is the most straightforward way to layer the calibration schedule selection and parameter update onto a characterization experiment, but there are other options that might give simpler calibration experiment definition at the expense of using more complicated code to set that up (like making BaseCalibrationExperiment a meta experiment (like ParallelExperiment) wrapping a BaseExperiment for example).

[chriseclectic]

I'm wondering if this method should be part of the Updater itself and be called like updater.update_calibrations(exp_data). Since experiments can define custom updaters they could overload the updater if needed. I'm not sure if there would be issues with this though since the examples in this PR are pretty straightforward so nothing overloads this method.

If this was changed we should probably add updater as a property to return the current updater too

[eggerdj]

I'd like to reconsider the way the updaters work in a subsequent PR focused more on the updaters.

[chriseclectic]

This blocking isn't needed now that #407 is merged. You should be able to do:

if self.auto_update:
    experiment_data.add_analysis_callback(self.updated_calibrations)

Which will have the update added to the async job so it runs after the analysis has finished

[wshanks]

Making it easy to queue multiple delayed updates to the same mutable Calibrations instance makes me a little nervous but we can see how it goes. Maybe one just gets used to calling block_for_results() when running calibrations.

[eggerdj]

Chris raised a good point that we do not necessarily want to enforce blocking when calibrating independent experiments on the same chip. For now we will have to get used to calling cals like this

exp1 = Exp1Cal(cals)
exp2 = Exp2Cal(cals)

exp1.run(backend).block_for_results()
exp2.run(backend).block_for_results()

when the experiments are dependent (e.g. Rabi then FineAmp on the same qubit) and like this

exp1.run(backend)
exp2.run(backend)

when they are independent (e.g. Rabi on qubit 0 and Drag on qubit 1).

[eggerdj]

See here: 88652ab

[chriseclectic]

LGTM!