Adapting Pulse Instructions to the new Pulse Model

[TsafrirA]

Summary

This PR proposes another option for the pulse Instructions adaptation to the new pulse model. The proposal is demonstrated on the Play instruction.

Details and comments

In Qiskit 1.0, the signature of the Play instruction is

def __init__(self, pulse, channel, name = None):

In #11219 we looked at ways to adapt the API to the new model, without introducing any breaking changes. The result was confusing at best. Here, we take a different approach which better utilizes the development model of Qiskit in the 1.0 era - because the new pulse model will officially take over at the earliest on Qiskit 2.0, we can utilize breaking changes to simplify the transition.

The option explored here, is to update the signature to

def __init__(self, pulse: Pulse, *, target = None, frame = None, mixed_frame = None, channel = None, name = None):

And then validate that exactly one of mixed_frame, channel or the duo target and frame were provided. By forcing keyword arguments, we avoid confusion and keep the code flexible going forward. Once we settle down on the exact format we want (deprecate Channels, decide on the correct order of arguments etc) we can remove the * operator without breaking anything.

Another benefit of this option, is the ability to introduce the new model in Qiskit 1.X. The signature could be updated to:

def __init__(self, pulse: Pulse, channel = None, name = None, *, target = None, frame = None, mixed_frame = None):

This doesn't break any existing code, while still allowing the user to transition to the new model. Upon transitioning to 2.0, the breaking changes are minimal - The user would simply need to add channel=... to the call to get his 1.X code to work with 2.0.

Some side effects should be noted -

• the _validate method loses its standing as the only place where inputs are validated. I actually think this is an opportunity to remove it altogether. From a documentation standpoint, the _validate method hides important information about the validation process (No "raises" part in the docs).
• Some properties are left here for compatibility, but it is likely that they can and should be removed once the compiler takes over (channel, channels etc).

Lastly, it should be noted that in #11219 there were comments about the need for all of the arguments presented here. While I disagree with the claim that the multiple options should be consolidated into a more basic model class, it could be argued that having three different options to instantiate the instruction is too much. (though it should be noted that going forward Channel will probably be removed).

In light of this criticism, we can explore yet another option with something like:

def __init__(self, pulse: Pulse, mixed_frame: Union[PulseChannel, MixedFrame, Tuple[PulseTarget, Frame]], name = None):

It has the benefit of reducing the number of arguments and the length of the call. The argument name change could be accommodated with deprecate_arg. It is a matter of personal preference I guess.

[wshanks]

I like the idea of providing a smooth transition. Looking at the options laid out here, I think like the last one.

def __init__(self, pulse: Pulse, mixed_frame: Union[PulseChannel, MixedFrame, Tuple[PulseTarget, Frame]], name = None):

because I don't see too much benefit to allowing target and frame as separate arguments if they must always be provided as a pair. Needing to pass them as a tuple to a single argument seems okay to me. That then makes me wonder about Thomas' point in https://github.com/Qiskit/qiskit/pull/11219/files#r1387928309 -- if it is worth the convenience to be able to do (target, frame) instead of MixedFrame(target, frame).

For a smooth transition, the signature could be

def __init__(self, pulse: Pulse, mixed_frame: Union[PulseChannel, MixedFrame, Tuple[PulseTarget, Frame]], name = None, channel: PulseChannel = None):

with the channel argument deprecated.

I edited the PR description to correct a couple typos (missing pulse argument from a couple examples).

[TsafrirA]

Thanks for the touchups @wshanks!
I have to admit that the last option I added was sort of an afterthought, but the more I thought about it, the more sense it made.

There is a reasonable argument to be made for the option presented in the PR code when you consider not only the Play instructions but also the other instructions. The Play instruction is actually unique in the sense that it's argument (no matter the way we frame it) has to be a mixed frame (=channel). The same is not true for other instructions. For example, a major reason for the new model is to be able to interact with frames and not only mixed frames. The ShiftPhase instruction can be defined with either on a frame or a mixed frame (=channel). We can go the same route there and have something like:

# ShiftPhase
def __init__(self, phase, frame: Union[Frame, MixedFrame, Channel]):

But I think it obscures the fact that inputting a Frame and a MixedFrame will lead to different behaviors down the line. When I imagined it, I thought that having the combo frame+target as arguments for all instructions creates a unified experience for the user. You could argue that a single argument could also be considered a unified experience. Perhaps with a clever name choice for that argument we can make it clear enough.

[wshanks]

That is a good point about the instructions that operate on frames rather than mixed frames. That makes it a little more reasonable to take target and frame as separate arguments to Play. Can you remind me why passing a Frame and MixedFrame to ShiftPhase has different results? I would expect passing a MixedFrame to be the same as passing MixedFrame.frame.

[TsafrirA]

Can you remind me why passing a Frame and MixedFrame to ShiftPhase has different results?

A ShiftPhase on a Frame will be broadcasted to all MixedFrames associated with that Frame. Imagine a QubitFrame which is associated, in terms of the legacy Channels, with both a DriveChannel and one or more ControlChannels.

[wshanks]

A ShiftPhase on a Frame will be broadcasted to all MixedFrames associated with that Frame. Imagine a QubitFrame which is associated, in terms of the legacy Channels, with both a DriveChannel and one or more ControlChannels.

And it is different for a ShiftPhase on a MixedFrame? Why would a ShiftPhase on a MixedFrame not be broadcasted to all the MixedFrames that shared the Frame used by the MixedFrame passed to ShiftPhase?

[TsafrirA]

And it is different for a ShiftPhase on a MixedFrame?

Yes, when applied to a MixedFrame the instruction will not be broadcasted, and instead be limited to the specific MixedFrame - the same way shifting the phase of DriveChannel won't shift the phase of associated ControlChannels. That way you have the flexibility to control an entire group of MixedFrames together, or target only a specific one.

[wshanks]

That way you have the flexibility to control an entire group of MixedFrames together, or target only a specific one.

The way that I think of a frame is as a frequency and phase versus time, independent of targets/outputs. In this picture, it doesn't make sense to shift the phase of the frame for one target relative to another using the same frame, though the angle values for individual pulses can be set. If you allow different targets to have different phases (or frequencies?) on the same frame, then the frame is more like a macro of common shifts to apply to the "real" frames which are local to each MixedFrame. I think that model can also work. It is just different from how I usually think of a frame.

[TsafrirA]

I see what you mean. I am used to thinking of Frame as something more abstract, and a MixedFrame as a specific NCO, so in principle the two can be controlled independently.

What do you think @nkanazawa1989 ?

[nkanazawa1989]

In my view the point of discussion is whether ShiftPhase can take MixedFrame. What @wshanks worries about would be the case like:

common_frame = Frame("my_frame")
mf = MixedFrame(common_frame, Qubit(0))

with build():
    shift_phase(pi, mf)
    play(my_pulse1, frame=common_frame, target=Qubit(0))
    play(my_pulse2, frame=common_frame, target=Qubit(1))

Semantically two pulses my_pulse1 and my_pulse2 are played at different phase but syntactically it says the frame is identical. I agree with @wshanks that this pattern is confusing. Since we target Qiskit 2.0 and breaking API change is allowed, maybe we can drop MixedFrame and Channel from frame-related instructions?

Note that we are just developing on the feature branch; We can in principle introduce as many changes as we want, and ask pulse users to test new pattern before we merge back to the main branch ;)

[TsafrirA]

That's a nice example @nkanazawa1989, though I doubt a user will specify it like that. If you choose to work with mixed frames, you'd probably do that all the way:

common_frame = Frame("my_frame")
mf = MixedFrame(common_frame, Qubit(0))

with build():
    shift_phase(pi, mf)
    play(my_pulse1, mixed_frame=mf)
    play(my_pulse2, frame=common_frame, target=Qubit(1))

I think written like that it's clearer. I also think we can consider individual mixed frame handling as an expert level use case. The more general thing will be to change the frequency\phase of Frame which will not cause any complexities.

About Channel - I think it's pretty aggressive to drop it in Qiskit 2.0. In any scenario, I don't think we can jump straight to the new model, without having a version where the two models coexist. In my opinion, a mixed model version is needed to allow users to transition.

[TsafrirA]

If there are no major objections, I propose to move forward with the option presented in this PR. I think it represents the broadest option, which will make subsequent structures equally sturdy. It will be easier to remove some options later on, than to add them retroactively across the entirety of the IR and compiler.

What do you think?

[nkanazawa1989]

LGTM. Let's try this pattern. I think we can add deprecation warning to channel and immediately drop this argument in 2.0. Then in the last minor release of 1.0 channel and mixframe will coexist for migration.

[qiskit-bot]

One or more of the the following people are requested to review this:

• @Qiskit/terra-core
• @nkanazawa1989

[TsafrirA]

Superseded by #11726.