Skip to content
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

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Conversation

eggerdj
Copy link
Contributor

@eggerdj eggerdj commented Jun 12, 2023

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, the Calibrations is based on the InstructionScheduleMap and it may be better to introduce a calibrations class that works with Target instead. This PR will allow us to, for example, and if desired

  • introduce a calibrations V2 class
  • gracefully deprecate calibrations in favor a class that is more aligned with recent developments in Qiskit

eggerdj and others added 26 commits April 29, 2022 14:55
Copy link
Collaborator

@nkanazawa1989 nkanazawa1989 left a 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.

eggerdj and others added 2 commits June 12, 2023 18:52
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
@itoko
Copy link
Collaborator

itoko commented Jun 13, 2023

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

@nkanazawa1989 nkanazawa1989 self-assigned this Jun 19, 2023
@eggerdj eggerdj added the on hold On hold until something else is done. label Jun 26, 2023
@eggerdj
Copy link
Contributor Author

eggerdj commented Jun 26, 2023

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:
Copy link
Contributor

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,

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold On hold until something else is done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants