-
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
Transpiling calibration experiments #952
Conversation
qiskit_experiments/calibration_management/base_calibration_experiment.py
Outdated
Show resolved
Hide resolved
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. I really like new pattern. Please write a upgrade release note because now transpile options are all ignored.
@@ -149,12 +155,12 @@ def _default_experiment_options(cls): | |||
result_index (int): The index of the result from which to update the calibrations. | |||
group (str): The calibration group to which the parameter belongs. This will default | |||
to the value "default". | |||
|
|||
add_measure_schedules (bool): A boolean that can be set to True (False is the default) |
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.
Can we do this automatically? Namely we can check the attached calibration instance if measure exist. To me such API seems to continuously grow and eventually out of control (like transpile options now).
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.
Removed it here: d5a21be for a follow-up PR.
qiskit_experiments/calibration_management/base_calibration_experiment.py
Show resolved
Hide resolved
qiskit_experiments/calibration_management/base_calibration_experiment.py
Outdated
Show resolved
Hide resolved
"""Attach measurement schedules to the circuit.""" | ||
for qubit in self.physical_qubits: | ||
meas_sched = self._cals.get_schedule("measure", qubit) | ||
circuit.add_calibration("measure", (qubit,), meas_sched) |
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 is not that simple. You must
- find measure op node, keep qreg-creg mapping separately
- get measure schedule from calibration
- iterate over all instruction blocks
- once you find an instruction with acquire channel then mutate memory slot index to match with the above mapping
This also allows us to avoid the creg conflict issue in parallel experiment. MemorySlot index can be always 0.
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.
Hmmm good point. Maybe worth having a PR focused around this and removing it from this PR. That will keep this refactor small.
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.
Removed it here: d5a21be for a follow-up PR.
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.
Issue here: #954
@abstractmethod | ||
def _attach_calibrations(self, circuit: QuantumCircuit): | ||
"""Attach the calibrations to the quantum circuit. | ||
|
||
This method attaches calibrations from the `self._cals` instance to the transpiled | ||
quantum circuits. Given how important this method is it is made abstract to force | ||
potential calibration experiment developers to implement it and think about how | ||
schedules are attached to the circuits. The implementation of this method is delegated | ||
to the sub-classes so that they can map gate instructions to the schedules stored in the | ||
``Calibrations`` instance. This method is needed for most calibration experiments. However, | ||
some experiments already attach circuits to the logical circuits and do not needed to run | ||
``_attach_calibrations``. In such experiments a simple ``pass`` statement will suffice. | ||
""" |
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.
@abstractmethod | |
def _attach_calibrations(self, circuit: QuantumCircuit): | |
"""Attach the calibrations to the quantum circuit. | |
This method attaches calibrations from the `self._cals` instance to the transpiled | |
quantum circuits. Given how important this method is it is made abstract to force | |
potential calibration experiment developers to implement it and think about how | |
schedules are attached to the circuits. The implementation of this method is delegated | |
to the sub-classes so that they can map gate instructions to the schedules stored in the | |
``Calibrations`` instance. This method is needed for most calibration experiments. However, | |
some experiments already attach circuits to the logical circuits and do not needed to run | |
``_attach_calibrations``. In such experiments a simple ``pass`` statement will suffice. | |
""" | |
def _attach_calibrations(self, circuit: QuantumCircuit): | |
"""Attach the calibrations to the quantum circuit. | |
This method attaches calibrations from the `self._cals` instance to the transpiled | |
quantum circuits. Given how important this method is it is made abstract to force | |
potential calibration experiment developers to implement it and think about how | |
schedules are attached to the circuits. The implementation of this method is delegated | |
to the sub-classes so that they can map gate instructions to the schedules stored in the | |
``Calibrations`` instance. This method is needed for most calibration experiments. However, | |
some experiments already attach circuits to the logical circuits and do not needed to run | |
``_attach_calibrations``. In such experiments a simple ``pass`` statement will suffice. | |
""" | |
pass |
As you wrote in the Rabi some experiments may not require calibration instance. Abstract method is one that all subclasses must define. You can do nothing by default, i.e. just pass
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.
Indeed. I purposefully avoided making this a default method with just pass
. The reason for this is that I want the developer to consciously think about how the calibrations are attached. If we go with pass
and make this a default method then the developer might forget to implement this which could result in undesired behavior which is hard to detect (e.g. the backend has all the default gates and the cals are not provided to override them).
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.
Fair enough.
qiskit_experiments/calibration_management/base_calibration_experiment.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/calibration_management/base_calibration_experiment.py
Outdated
Show resolved
Hide resolved
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.
LGTM
|
||
def _attach_calibrations(self, circuit: QuantumCircuit): | ||
"""Adds the calibrations to the transpiled circuits.""" | ||
schedule = self._cals.get_schedule("x", self.physical_qubits) |
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 didn't have a chance to review the PR before merging. My one minor comment is to wonder if this should be optional. Perhaps someone would want to calibrate the 12 gates while relying on the default 01 calibrations?
Summary
This PR refactors the internal transpilation of the calibration experiments.
Details and comments
Calibration experiments are designed with a specific gate sequence in mind. Running the transpiler on them in a generic fashion creates a risk that the circuits are altered in a fashion that the experiment no longer does what it is intended to do. E.g. by swap insertion, basis gate unrolling, etc.. This PR therefore defines a minimal framework needed in calibration experiments to transpile the circuits to the backend. This PR has the added benefit that we no longer need to rely on the instruction schedule map.
Tutorial notebook also runs well. Before cals:
After cals