-
Notifications
You must be signed in to change notification settings - Fork 61
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
Qiskit Pulse migration to Dynamics #365
base: main
Are you sure you want to change the base?
Qiskit Pulse migration to Dynamics #365
Conversation
See Qiskit/qiskit#13063 also |
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 for migration of the pulse module! This is great job :)
I removed one that had to do with Qobj at pulse level, I'm not sure what is the future for such interface in Qiskit core
This should be fine because Qiskit Dynamics doesn't require wire format. These are missing components in the PR:
- Please write release note
- Please also migrate the visualization module (
qiskit.visualization.pulse_v2
)
May main concern is usage of monkey patch as I wrote in the comments below.
qiskit_dynamics/pulse/__init__.py
Outdated
|
||
r""" | ||
=========================== | ||
Pulse (:mod:`qiskit.pulse`) |
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 think this is not rendered as a module documentation. You also need to update Sphinx API docs configuration.
https://github.com/qiskit-community/qiskit-dynamics/blob/main/docs/apidocs/index.rst
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.
Not sure how to proceed for this. I have modified the Python file to make it more coherent as a whole but did not touch the docs yet.
@@ -22,7 +22,7 @@ | |||
import numpy as np | |||
import sympy as sym | |||
|
|||
from qiskit.pulse import ( | |||
from qiskit_dynamics.pulse import ( |
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.
It would be great if you can directly convert ScheduleBlock
into Signal
to remove the legacy Schedule
along with this module. Of course this is not the scope of this PR. Just keep it in mind :)
Perhaps we can move pulse_to_signals
under transforms
submodule.
if name: | ||
cpy.name = name | ||
return cpy | ||
if not hasattr(QuantumCircuit, "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.
I don't think monkey patch is not right approach because of following reasons:
-
Monkey patch is a temporary solution and should not be permanent. Qiskit core decided to drop these methods permanently.
-
This hack affects not only QiskitDynamics but also other packages such as add-ons using
QuantumCircuit
object. These packages are not aware of the hack and might cause unexpected interactions (edge cases). This is almost unmanageable. -
If someone wants to write new package with the calibration feature, it's difficult to know when this monkey patch is applied. One may need to write
hasattr(circ, "calibrations")
every time, but this is very cumbersome. -
If you transpile this patched circuit object, the output is the same
QuantumCircuit
type but._calibrations
data is lost because the dag to circuit conversion doesn't care the calibration data. From the end user viewpoint, this behavior looks a bug. But we don't have bandwidth to maintain the custom transpile infrastructure.
Instead of using monkey patch, I would define an explicit subclass with multiple inheritance not to tightly couple calibration data to the core data model of QuantumCircuit
.
For example:
from qiskit.circuit import QuantumCircuit
from functools import wraps
class Calibration:
@wraps(QuantumCircuit.__init__)
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.calibrations = {}
@wraps(QuantumCircuit.assign_parameters)
def assign_parameters(self, *args, **kwargs):
super().assign_parameters(*args, **kwargs)
... # implement assign logic
class CalibratedCircuit(Calibration, QuantumCircuit):
@classmethod
def from_circuit(cls, circuit):
new_circ = circuit.copy()
new_circ.__class__ = cls
new_circ.calibrations = {}
return new_circ
With this, the signature of transpile is
transpile(QuantumCircuit|CalibratedCircuit) -> QuantumCircuit
and user can notice transpile doesn't support calibration.
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 for this review. I like the proposed solution. However, wouldn't there be a way to replicate the Analysis and Transformation passes that were introduced in the scheduling stage, and add them as plugins of the DynamicsBackend object? I was moreover wondering if those custom backend plugins as described here (https://docs.quantum.ibm.com/api/qiskit/transpiler_plugins) could be automatically called when using transpile() over the backend. If yes, then we could just adapt the case of the transpilation pipeline to take the plugins from the backend, checking for calibrations (we would therefore check that the type of the input is a CalibratedCircuit
. What do you think?
Summary
This PR is a first step towards Qiskit Pulse migration to Qiskit Dynamics.
I
Details and comments
t contains: