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

Transpiling calibration experiments #952

Merged
merged 24 commits into from
Oct 26, 2022

Conversation

eggerdj
Copy link
Contributor

@eggerdj eggerdj commented Oct 25, 2022

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:

image

After cals

image

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. 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)
Copy link
Collaborator

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

Copy link
Contributor Author

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.

"""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)
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue here: #954

Comment on lines +289 to +301
@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.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@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

Copy link
Contributor Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

@eggerdj eggerdj changed the title [WIP] Transpiling calibration experiments Transpiling calibration experiments Oct 26, 2022
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.

LGTM

@eggerdj eggerdj merged commit 1a600a7 into qiskit-community:main Oct 26, 2022
@eggerdj eggerdj deleted the base_cal_transpil branch October 26, 2022 19:23

def _attach_calibrations(self, circuit: QuantumCircuit):
"""Adds the calibrations to the transpiled circuits."""
schedule = self._cals.get_schedule("x", self.physical_qubits)
Copy link
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants