-
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
Support frames using signals #5977
Conversation
…ts in the builder.
* Made left and right alignment contexts sensitive to frames.
… in resolve_frames.
* Modified docstring.
* Added basic unittests.
…supported for backward compatibility.
* Shift/Set instructions can return a channel if the frame implies one. * Removed the ignore frames logic in the builder.
* Added test for requires_mapping.
* Added frames_config conversion to dict.
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 an exhaustive review at all, just came across this PR and was trying to understand what it will mean for future.
I work with ions, and this concept is critical to how we talk about our multi-qubit gates (typically Molmer-Sorensen): they are defined in terms of the motional and spin phases. I think this PR is great because it (hopefully) allows us to control the motional & spin phases at a higher level than the physical oscillator channels.
A few questions:
- Does this allow for mapping a single frame to multiple physical channels? i.e.
Frame("motion", 0) -> ((freq=200 MHz, phase = 0, channel = DriveChannel(0)), (freq=190 MHz, phase = 180, channel=DriveChannel(1))
- How does it handle multiple frames played on the same channel? Are frames mutually exclusive on the same hardware channel? I'm envisioning e.g. some parallel cooling schemes that play several tones simultaneously on different channels. e.g. supporting hardware that has many output tones on a single hardware output channel (https://m-labs.hk/thesis_nkrackow.pdf, Sec 3.2.1, 1024 tones (frames) per channel simultaneously!). I've been working ondeveloping an OpenPulse backend for a two-simultaneous-tone-per-channel output device, and my workaround was just to have a separate DriveChannel per each tone.
@@ -232,6 +233,15 @@ def channels(self) -> Tuple[Channel]: | |||
"""Returns channels that this schedule uses.""" | |||
return tuple(self._timeslots.keys()) | |||
|
|||
@property | |||
def frames(self) -> Tuple[Frame]: |
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.
A tuple implies ordering, but the set that the return value is generated from is unordered, so the .frames
value can change its ordering over time. IMHO it makes sense to just return the explicitly unordered Set[Frame]
.
) -> Schedule: | ||
"""A basic pulse program transformation for OpenPulse API execution. | ||
|
||
Args: | ||
sched: Input program to transform. | ||
remove_directives: Set `True` to remove compiler directives. | ||
frames_config: The Configuration with which to resolve any frames. |
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.
frames_config: The Configuration with which to resolve any frames. | |
frames_config: The FramesConfiguration with which to resolve any frames. |
@@ -179,7 +170,7 @@ def test_substitution(self): | |||
def test_substitution_with_existing(self): | |||
"""Test that substituting one parameter with an existing parameter works.""" | |||
schedule = pulse.Schedule() | |||
schedule += pulse.SetFrequency(self.alpha, DriveChannel(self.qubit)) | |||
schedule += pulse.SetFrequency(self.alpha, DriveChannel(self.qubit).frame) |
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 expectation of changing to Channel(i).frame
should be explicitly documented somewhere.
class FrameDefinition: | ||
"""A class to keep track of frame definitions.""" | ||
|
||
# The frequency of the frame at time zero. |
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.
These docstrings can be converted to attribute docstrings by moving after the attribute declaration. https://www.python.org/dev/peps/pep-0224/
# The phase of the frame at time zero. | ||
phase: float = 0.0 | ||
|
||
# Tolerance on phase and frequency shifts. Shifts below this value are ignored. |
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.
Include units: degrees or radians?
Hi @drewrisinger Thanks for sharing the valuable feedback from the ion system. Regarding 2, I think this should be supported because this is quite useful to qutrit or qudit type experiment. Considering case1, we should be able to cope with N:M mapping problem, which is very complicated based on my experience. However it sounds like very important since Qiskit is hardware agnostic SDK. Anyways the issue of current implementation is poor compatibility with pulse gates which we are strongly encouraging, i.e. frame-resolved schedule cannot be attached to a circuit because its phase is timing-dependent. This means backend/provider should support frame syntax, and this makes me think end-users cannot use arbitrary frame except for one defined by the backend. Maybe we need to start from defining the usecase of frames. Actually current frame-resolving logic is computationally expensive. I didn't take accurate benchmark, but scheduling a set of 1k depth circuits with frame-contained instruction takes 10s of minutes on my laptop. So pulse gate support is necessary if we want to use custom pulse schedule in some practical algorithms. |
@drewrisinger I think you might be confusing how the terms "signal" and "frame" are being set up in this PR. A frame here is a frequency and phase pair that can be modified over time and is independent of channel, and a signal is a play instruction combined with a frame. So your construction here doesn't make sense to me: Signals still get played on channels, so frames don't change the fact that you can only have one play instruction at a time on a PulseChannel. Your workaround of creating extra DriveChannels to play simultaneous pulses is a good way to do simultaneous pulses when you control the backend. The problem is that that isn't always the case -- like in @nkanazawa1989's example of a qutrit where the backend might be set up with one DriveChannel per qubit but you want to drive other transitions on those qubits. It would be nice if one could define virtual channels that get combined into simultaneous plays on a single DriveChannel by the backend. That would be a different feature from frames and signals though. It is related though -- frames could be tied to such virtual channels and then one would not need to use the Like @nkanazawa1989 mentioned, this PR is somewhat paused because more backend support for frames is needed for it to really work well. |
I agree. The solution is to either require that a frame+channel is a virtual channel, with its own timing or have a separate construct which is |
I also prefer the virtual channel model. I've tested the model in my private code, and indeed this model has a good compatibility with the pulse syntax, i.e. we just need to upgrade the backend |
Summary
Qiskit pulse currently lacks the concept of frames this PR introduces a frame concept in which SetFrequency, ShiftFrequency, SetPhase, and ShiftPhase instructions can be applied to a frame which corresponds to a frequency and a phase. The PR defines a Signal class which corresponds to a Pulse in a given frame.
Details and comments
This PR explores the syntax
Several key changes are introduced:
Frame
is introduced. It takes a prefix and an index, e.g.Frame("d", 123)
has the named123
. A frame may correspond to a PulseChannel if it has the right prefix (d
forDriveChannel
,u
forControlChannel
, etc).SetFrequency
,ShiftFrequency
,SetPhase
, andShiftPhase
no longer operate onPulseChannel
but operate on instances ofFrame
.chan
is associated a native frame given byFrame(chan.prefix, chan.index)
.Consider the example of a qutrit:
Here, the second pulse is played in the frame of the qutrit. This pulse will be mapped to the frame of the qubit, i.e.
Frame("d", 0)
, using the appropriate shift/set instructions whenmap_frames
is called. Once frame resolution (now called mapping) is done the schedule will look like