-
Notifications
You must be signed in to change notification settings - Fork 127
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
Base calibrations #1200
base: main
Are you sure you want to change the base?
Base calibrations #1200
Conversation
…s into base_calibrations
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 Daniel. Overall this looks good to me, but the standpoint of this base class is bit unclear. I think this abstract class must define attributes and methods which are used by calibration experiments, so that we can safely implement new calibration agent without breaking experiment library. In this sense, this could be also a Protocol.
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Thanks @eggerdj. It makes sense to me to have an interface class (BaseCalibrations) for calibration experiment classes for the smooth deprecation of current Calibrations class in the future. That said, the interface will last long and possibly affect the implementation of unseen another Calibrations (other than V2). I think we have enough time to explore a better interface and don't need to rush to merge it. So I'd like to see a concrete example of calibrations V2 class first. It is difficult for me to check if an abstract class is well-designed without having at least two subclasses. Also I think BaseCalibrations don't need to be just a subset of the existing Calibrations. It may be a good timing to consider minor improvement of Calibrations through following the new interface (I think Naoki's suggestions indicate that). |
Marking as "On Hold" until we get more certainty on the required interface. |
) -> Union[int, float, complex]: | ||
"""Retrieves the value of a parameter. | ||
|
||
Parameters may be linked. :meth:`get_parameter_value` does the following steps: |
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.
Note that this sentence is not finished. Also, what do you mean with "Parameters may be linked"? There's only one parameter here,
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.
This indicates parameter update can affect other schedules since it's linked.
Summary
This PR introduces the BaseCalibrations abstract base class.
This class introduces the minimal needed interface for calibration experiments to work.
Details and comments
This change is necessary as it will allow us to pursue the development of calibrations and keep ourselves aligned with developments in Qiskit. For example, the
Target
is the preferred way of creating backends. Currently, theCalibrations
is based on theInstructionScheduleMap
and it may be better to introduce a calibrations class that works withTarget
instead. This PR will allow us to, for example, and if desired