-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Serializable parametric pulse #7821
Serializable parametric pulse #7821
Conversation
This PR makes following changes. 1. add `_define` method to ParametricPulse. This method is expected to be implemented by each ParametricPulse subclass. Subclass must return Sympy or symengine symbolic equation that can be serialized. This method is called once in the constructor to fill `.definition` attribute of the instance. This definition is used for QPY serialization. 2. change behavior of `get_waveform` This method is originally implemented as abstractmethod that calls another callback that generates numpy array of samples (thus not serializable). Now this is a baseclass method that generates waveform array from the symbolic equation. 3. minor updates Now pulse parameters are not direct class attribute. Parameter names are defined as class attribute `PARAMS_DEF` and values are stored as instance variable `param_values`. This is good for writing serializer since it doesn't need to know attribute of each subclass, but just can call name def and values to get data to serialize.
This works around an issue with sympy where its lambda returns a scalar instead of an array when the expression evaluates to a scalar: sympy/sympy#5642
…ulse Improve performance of parametrized pulse evaluation
Co-authored-by: Will Shanks <willshanks@us.ibm.com>
…ion for better integration with circuit instruction
# which causes performance issue in the lambda function. | ||
# In addition, there are several unsupported expression of boolean operation. | ||
# Thanks to Lambdify at subclass instantiation, the performance regression is not significant here. | ||
import sympy as sym |
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 needs to be a runtime import to avoid an import performance regression when the parametric_pulses
module gets imported.
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.
One thing to note -- ParametricPulse.__init_subclass__
uses lambdify_symbolic_pulse
which uses some sympy
functions. So even if the sympy usage was shifted to look like it was a run time import, sympy would still get imported when this module is loaded because of the subclasses like Gaussian
which are defined here (and imported in qiskit.pulse
's __init__.py
). At the cost of some extra complexity, this sympy usage could be shifted from class definition to first usage.
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.
If you can shift it to first-use, not import, that would be better - sympy
is a super heavy import, so we need to make sure it's not in any code paths that are run during import qiskit
.
I think it shouldn't be too much additional complexity; you can either put an extra field on the class to store the cached value and make definition
a regular property that returns the class-level cache or populates it as necessary, or you can write a custom descriptor that does essentially the same thing, but stores the cached value in a separate object to keep the main type clean.
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 @nkanazawa1989 was pushing the change to first-use literally as you were typing submitting (just read the comment about your review being old) this 🙂
qiskit/pulse/utils.py
Outdated
|
||
import numpy as np | ||
import sympy |
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.
Same thing here this should be a runtime import
As discussed in qiskit-terra issue Qiskit#7659, the definition of the LiftedDrag pulse was not consistent with its implementation in qiskit.pulse.library.continuous.py. One error is fixed by this PR. A remaining problem is the explicit definition of the sampler strategy. This should be fixed after/with the update of parametric pulses, qiskit-terra issue Qiskit#7821.
qiskit/pulse/utils.py
Outdated
# # lambdify fully supports the features required by parametric | ||
# # pulses. | ||
# import symengine | ||
# expr = symengine.sympify(expr) |
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.
We might want to leave a symengine version here in a comment for reference for once piece-wise support is available. In nkanazawa1989#48, I had mentioned that symengine did not support complex numbers. It turns out it does, but you need to pass real=False
to Lambdify
.
So sympy.lambdify(symbols, expr)
becomes symengine.lambify(symbols, [expr], real=False)
.
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 Will for investigation. I didn't realize that option. Probably we can re-add symengine but I need more investigation for boolean expressions.
* Correct documentation of Drag/LiftedDrag As discussed in qiskit-terra issue #7659, the definition of the LiftedDrag pulse was not consistent with its implementation in qiskit.pulse.library.continuous.py. One error is fixed by this PR. A remaining problem is the explicit definition of the sampler strategy. This should be fixed after/with the update of parametric pulses, qiskit-terra issue #7821. * Implement review update of #7856 - Removal of f'(x) - Added definition of g'(x) - Typos corrected 'gaussian' -> 'Gaussian' - Link to `qiskit.pulse.library.Gaussian` added * Fixing linting errors #7856 Fixed the linting errors (line too long) in lines 353, 356, and 358 of qiskit/pulse/library/parametric_pulses.py. * Replace URL link to Gaussian Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Removal of trailing whitespace in line 357 Moved link to Gaussian via :class: from line 358 to 357. Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* Correct documentation of Drag/LiftedDrag As discussed in qiskit-terra issue #7659, the definition of the LiftedDrag pulse was not consistent with its implementation in qiskit.pulse.library.continuous.py. One error is fixed by this PR. A remaining problem is the explicit definition of the sampler strategy. This should be fixed after/with the update of parametric pulses, qiskit-terra issue #7821. * Implement review update of #7856 - Removal of f'(x) - Added definition of g'(x) - Typos corrected 'gaussian' -> 'Gaussian' - Link to `qiskit.pulse.library.Gaussian` added * Fixing linting errors #7856 Fixed the linting errors (line too long) in lines 353, 356, and 358 of qiskit/pulse/library/parametric_pulses.py. * Replace URL link to Gaussian Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Removal of trailing whitespace in line 357 Moved link to Gaussian via :class: from line 358 to 357. Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit c90c176)
* Correct documentation of Drag/LiftedDrag As discussed in qiskit-terra issue #7659, the definition of the LiftedDrag pulse was not consistent with its implementation in qiskit.pulse.library.continuous.py. One error is fixed by this PR. A remaining problem is the explicit definition of the sampler strategy. This should be fixed after/with the update of parametric pulses, qiskit-terra issue #7821. * Implement review update of #7856 - Removal of f'(x) - Added definition of g'(x) - Typos corrected 'gaussian' -> 'Gaussian' - Link to `qiskit.pulse.library.Gaussian` added * Fixing linting errors #7856 Fixed the linting errors (line too long) in lines 353, 356, and 358 of qiskit/pulse/library/parametric_pulses.py. * Replace URL link to Gaussian Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Removal of trailing whitespace in line 357 Moved link to Gaussian via :class: from line 358 to 357. Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit c90c176) Co-authored-by: Tobias Kehrer <99488672+tobias-kehrer@users.noreply.github.com>
…rializable-parametric-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 just opened this PR, and apparently I wrote these review comments about a month ago and they never submitted properly. I'm sorry if they're a bit out-of-date or incomplete - I just wanted to push them through rather than leaving them in GitHub limbo.
# which causes performance issue in the lambda function. | ||
# In addition, there are several unsupported expression of boolean operation. | ||
# Thanks to Lambdify at subclass instantiation, the performance regression is not significant here. | ||
import sympy as sym |
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.
If you can shift it to first-use, not import, that would be better - sympy
is a super heavy import, so we need to make sure it's not in any code paths that are run during import qiskit
.
I think it shouldn't be too much additional complexity; you can either put an extra field on the class to store the cached value and make definition
a regular property that returns the class-level cache or populates it as necessary, or you can write a custom descriptor that does essentially the same thing, but stores the cached value in a separate object to keep the main type clean.
cls.constraints = [lambdify_symbolic_pulse(c, params) for c in cls._constraints()] | ||
else: | ||
cls.constraints = sym.true |
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.
The type of cls.constraints
here looks a bit inconsistent - in one branch it's a list of functions, in the other it's the Sympy truth constant. Given that (I think) you usually use this by effectively all(c(params) for c in cls.constraints)
, the global truthiness state could also just be represented by the empty sequence.
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.
Make sense. Done in 2ea6390. I intentionally avoid using all(c(params) for c in cls.constraints)
or something like map
for better error message. It still checks constraints one by one to tell users which one failed.
amplitude is constrained to 1. | ||
waveform to 1. The default is ``True`` and the | ||
amplitude is constrained to 1. | ||
type: Type of this waveform. This appears in the representation string. |
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's pretty odd to pass this as a parameter - is there a reason not to use type(self).__name__
in the repr?
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.
Ah good catch! I was first thinking how we can QPY deserialize custom pulse class based on envelope and constraints symbolic equations. I was trying to mimic circuit Gate
in which they use common base class with custom instance-level definition and unique name to represent custom instruction. So I was thinking to use this type
as name
(but in pulse module it should behave as class name).
Anyways we decided to add these symbolic definitions to class attribute (to avoid lambdify overhead that the circuit module doesn't have) so this idea doesn't work, i.e. we cannot define two different waveforms on top of the SymbolicPulse
base class.
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.
In this sense probably your idea of descriptor #7821 (comment) makes this framework much cleaner. In the current implementation we need to dynamically generate custom Type in the QPY loader.
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.
Descriptor would be the best solution 0738497. We can QPY serialize the state of descriptor since it can store definitions of all pulse types. Then we can directly restore lambda functions before loading actual pulse instructions. Likely this will simplify the framework for loading custom pulses.
590a297
to
15e9f7c
Compare
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.
Looks good to me 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.
Overall this looks great to me. I'm glad that @wshanks did a detailed review as he'll be a heavy user of this and can provide better feedback on the interface. I've left a few inline comments mostly on documentation but nothing major.
The only open question for me really is if we wanted to have dedicated classes for the SymbolicPulse
class, especially to test the validation and error cases. You have tests via the library pulse classes which give some coverage, do you think that's sufficient?
@@ -356,21 +384,21 @@ def __init__( | |||
) | |||
self._amp = parameters.pop("amp") | |||
self._pulse_type = pulse_type | |||
self._param_names = tuple(parameters.keys()) | |||
self._param_vals = tuple(parameters.values()) | |||
self._params = parameters |
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.
If you want truly an immutable type that's really tricky to do in python. You can create one like frozendict pretty easily without that library just something that implements __getitem__()
and the other read only mapping protocol functions will work. Basically just subclass collections.abc.Mapping
. You could also do it via rust via a HashMap
or IndexMap
rust struct if you wanted something with statically typed keys which performed better. But that only limits it to top level immutability (basically only blocking inserts and value replacements) you'll always still be able to modify a value inplace. For example, using frozendict you could do something like:
import frozendict
test = frozendict.frozendict({'a': []})
test['a'].append(2)
print(test)
Would print: frozendict.frozendict({'a': [2]})
releasenotes/notes/add-serializable-parametric-pulse-31490c4d2cc49ec6.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/add-serializable-parametric-pulse-31490c4d2cc49ec6.yaml
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 for the updates I think with one small doc update about the platform support this is good from my end too.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
The support for serializing SymbolicPulse objects using QPY was added in PR Qiskit#7300 and has been released as part of 0.21.0. However, the documentation for the SymbolicPulse class had a note saying the QPY support was pending. This was necessary when the SymbolicPulse class was added in Qiskit#7821 because it was unclear when we'd be able to add the QPY support. But, Qiskit#7300 was updated and merged soon after Qiskit#7821, but we neglected to remove that note. This commit corrects the oversight and removes the stale note.
The support for serializing SymbolicPulse objects using QPY was added in PR #7300 and has been released as part of 0.21.0. However, the documentation for the SymbolicPulse class had a note saying the QPY support was pending. This was necessary when the SymbolicPulse class was added in #7821 because it was unclear when we'd be able to add the QPY support. But, #7300 was updated and merged soon after #7821, but we neglected to remove that note. This commit corrects the oversight and removes the stale note. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
The support for serializing SymbolicPulse objects using QPY was added in PR #7300 and has been released as part of 0.21.0. However, the documentation for the SymbolicPulse class had a note saying the QPY support was pending. This was necessary when the SymbolicPulse class was added in #7821 because it was unclear when we'd be able to add the QPY support. But, #7300 was updated and merged soon after #7821, but we neglected to remove that note. This commit corrects the oversight and removes the stale note. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 47c7c27)
…#8292) The support for serializing SymbolicPulse objects using QPY was added in PR #7300 and has been released as part of 0.21.0. However, the documentation for the SymbolicPulse class had a note saying the QPY support was pending. This was necessary when the SymbolicPulse class was added in #7821 because it was unclear when we'd be able to add the QPY support. But, #7300 was updated and merged soon after #7821, but we neglected to remove that note. This commit corrects the oversight and removes the stale note. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 47c7c27) Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
This is overhaul of
qiskit.pulse.library.parametric_pulse
. This is necessary for QPY support of pulse schedules.Background
QPY mechanism allows us to serialize Qiskit programs with better backward compatibility. Currently this only supports
QuantumCircuit
but we should supportScheduleBlock
andSchedule
for:QuantumCircuit.calibrations
QiskitExperiments
These pulse programs are made of pulse instructions, such as
Play
,ShiftPhase
,ShiftFrequency
, etc... and they take instruction operands and channels. These objects should be serializable, however, thePulse
instance that is an operand of thePlay
instruction cannot be serialized in every situation. See the discussion here for details.The
Pulse
instruction has two classes,Waveform
andParametricPulse
, and the pulselibrary
defines four sub-classesGaussian
,GaussianSquare
,Drag
, andConstant
. IBM Quantum backends define the same classes, and we just need to submit only parameter values and pulse type information through the communication layer. This drastically saves data volume we need to transmit over the internet. However this mechanism is not scalable and has several downsides.Waveform
. Because this is just a sequence of complex data points, we are no longer able to infer raw parameter values, such asamp
,phase
, etc... which is quite important in the context of system monitoring and maintenance. Also this yields significant increase of data volume and hurts performance of could computing. Note that we are already lacking the definition of DDCX pulse in Qiskit.ParametricPulse
, to make the database serializable. Again, if Qiskit doesn't support a particular pulse shape, an experimentalist must commit to Qiskit with new pulse shape. This may unnecessarily expose IP before publication or invention disclosure (if one will).To overcome, every
ParametricPulse
subclass should be serializable even though it is not defined in Qiskit.Challenges
All
ParametricPulse
must defineget_waveform
method. When this method is called, an instance should generate numpy array of waveform with instance's pulse parameters. Currently, the program to generate waveform is hard-coded there. Note that, in general, serialization of arbitrary python callback is tough challenge.All
ParametricPulse
must definevalidate_parameters
method. This is called at the constructor when all parameters are bound. This should raise an error if there is a risk of invalid waveform generation. For example, if maximum value of the waveform exceeds 1.0, it raises anPulseError
. Note that this is also a set of callables.https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/pulse/library/parametric_pulses.py
New mechanism
Here we use
Sympy
symbolic expression instead of callables. These symbolic expressions are usually serializable, thus we should be able to QPY serialize full pulse schedule containing fancy pulses under the research. The upgradedParametricPulse
implementation conforms to that of circuitGate
structure.The class
definition
andconstraints
are initialized at__init_subclass__
method when the class is initialized with lambdify. Because in typical pulse programs, the same waveform are repeatedly used with different parameters (for different qubits), it is important to cache the lambda functions without having parameters assigned at the class level. Usually the waveform generation with symbolic equation is heavy computing overhead without lambdify. This is due to long waveform samples and complexity of equation. This often hurts visualization of schedules, where we need to convert allParametricPulse
s toWaveform
. @wshanks investigated this mechanism carefully and optimized. Now the overhead of waveform visualization is almost comparable to one in the main branch.Co-authored-by: Will Shanks willshanks@us.ibm.com
TODO
Drag
pulse validation whenbeta=0
(see ITE evaluates all args at the same time sympy/sympy#23295)Consideration of symengine (we had long discussion but I dropped with c4b41d5 owing to the inconsistent behavior in Boolean logic)In follow-up
ParametricPulse
, e.g.DDCX
)