Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 70 commits
afe4f96
9a8a582
6f2496a
0424636
1bf11fa
2335f32
2af235d
0cc7383
a59af77
21ef762
509470a
ed2f661
c4b41d5
71ef841
cf7f302
c2981d0
cf17341
ba685ef
f2ef950
2ea6390
f48796e
0738497
92f42eb
1440a23
4345c3b
1c63ddb
d121b0d
48ff3cc
b9cab67
42b3dd9
761235d
cc12bf8
6c21edd
43e0e0b
6b956de
eefa5e4
252f62b
f24e648
4d191be
2c8e7f8
3a0f506
d5517d5
be2ed0c
67ac61f
78288ac
cbada64
ecb4ea7
45ae755
a4a56db
d957458
6776743
d4e8c43
41bf4f5
2aa4f5a
2e521a7
09855d7
b99eb5c
ac23d6f
401c1f4
686e77b
84344c2
914018e
045d67b
3b2c571
8edaba9
9b0adc3
cda7336
ef35cd9
15e9f7c
d844253
526aaed
87d3538
be2b4e0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Replacing
ParametricPulse
withSymbolicPulse
brings several issues to mind for me. I haven't settled on what I think is best regarding them:SymbolicPulse
allows for a future where a symbolic pulse expression is sent to the backend and rendered there. However, at the moment, all that is supported are the same possibilities asParametricPulse
now -- either sending the name and parameter values or falling back to the a sample pulse.ParametricPulse
and keeping theParametricPulse
class. That might be more trouble than it's worth, though it seems likeParametricPulse
subclasses need to override all the important methods any way, so they would almost already work if they inherited fromSymbolicPulse
instead.ParametricPulse
for this case, though I was surprised to realize only specific classes inParametricPulseShapes
are allowed to be passed to the backend. I would have expected to be able to pass a pulse name and set of parameters and as long as the pulse name was in the backend's run config for it to be okay.What I am a little concerned about is this disconnect between fully specifying a pulse with a symbolic expression and specifying a pulse as a name and set of parameters that is implemented by the backend. Is it best to try to blur the line between those two or keep them explicitly two separate things? Serialization is not in this PR, but another thing I wonder about is if Gaussian, for example, would be serialized as a symbolic expression and deserialized as a generic symbolic pulse or if it would be serialized as a "gaussian" pulse and then deserialized back to a Gaussian.
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.
Data format in the communication layer is one of the important aspects of the symbolic pulse, however, for us, the most important feature is the serialization of calibration database, where users may actively introduce new pulse shapes with new calibration techniques.
I tend to disagree with supporting both
ParametricPulse
andSymbolicPulse
from viewpoint of maintenance overhead. This means we need to keep double amount of unit tests. Since we already haveSchedule
andScheduleBlock
, this will make situation more complicated, i.e. there are 4 different combination of data models to create a pulse program.This is why we need symbolic pulse. Indeed how this code is used is unclear now, because job submission mechanism is totally offloaded to provider. Qiskit just passes python object to the provider. If one introduces new pulse shape in the backend config, he/she should update the provider code together with parametric pulse library in Qiskit. And user need to wait for next release to enjoy backend-supported parametric pulse. I think this is why DCX (target) pulse is still provided as waveform.
I agree keeping current mechanism of name + parameters for predefined pulses. This will help us to reduce data volume of QPY binary. Regarding the serialization, I plan to write symbolic equations in the program header, instead of duplicating it for every appearance of play instruction. For example, in the circuit we have
https://github.com/Qiskit/qiskit-terra/blob/04807a13a7ba53d97c37bb092324e419296e3fba/qiskit/qpy/binary_io/circuits.py#L591-L607
If we only save equations of custom pulses, we can compactly represent the program (i.e. DRAG is very often used but constraints is quite big object). Same mechanism exists in circuit IO.
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 backend configuration includes a
parametric_pulses
entry that lists the supported parametric pulses.assemble_schedules
currently gets the type of thePulse
in eachPlay
instruction and translates it to a string with theParametricPulseShapes
enum. Then it checks if this string is in the backendparametric_pulses
.Here is a suggestion for how things could work differently to decouple the provider from terra:
pulse_type
instead of the class name, sogaussian_square
instead ofGaussianSquare
, to match the format ofparametric_pulses
in the provider.assemble_schedules
could check ifpulse.pulse_type
is the backend'sparametric_pulses
directly without doing a translation through theParametricPulseShapes
enum.Then a provider could add support for additional pulses and the user would just need to define
SymbolicPulse
instances with the rightpulse_type
but would not need to modify terra.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.
Yeah I was thinking to use
pulse_type
for this purpose but this induces a breaking API change. Because this camel case name is used everywhere, e.g. printing schedule would giveIf the symbolic pulse needs to use conventional names in
parametric_pulses
, then this will becomewhich may confuse the users. This also changes labels in the pulse visualizer and
repr
of pulse instances. Also we still need this Enum to convert backend's CmdDef back to the schedules (i.e. construction of instmap) because we need to extract envelope expressions from somewhere in the pulse module unless the backend replaces CmdDef with QPY schedules. I agree this approach is still effective to simplify the logic of schedule -> Qobj.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.
Hmm, so what is a path forward for not requiring pulses to be hardcoded in terra for them to be passed parametrically to the backend? Wait for a backend and provider that supports QPY jobs and arbitrary symbolic pulses?
For backwards compatibility we could store
GaussianSquare
andgaussian_square
separately inSymbolicPulse
and use one for display and one for checking for backend support.I don't see a way to avoid keeping the enum defined for now for the instruction schedule map usage. Perhaps we could get backends to start publishing the CmdDef as
SymbolicPulses
in the future.