Skip to content
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

[RFC] Unify Pulse Commands and Instructions #12

Conversation

taalexander
Copy link
Collaborator

Commands were initially created so as to allow pulses to be only defined once as a SamplePulse and then have their usage tracked based on the class instance through equality checking. To enable this, every Instruction instance was defined as containing a Command and its Channel operands that the command would be applied to. This resulted in a confusing API as one must first define a command and then call it with a channel to emit an instruction. We propose a way of gracefully unifying Commands and Instructions to make pulse programming more straightforward and in-line with traditional instruction sets.

The implementation of this RFC will be tracked by the Terra issue

@lcapelluto
Copy link

Comment on names: These are used for visualization, so, if pulses and instructions both have names, one becomes unused, so I would prefer only one of the two to have a name. Between the two, it seems to make more sense for instructions to have names (or else non pulses like PhaseShift don't get a name). I don't really see the downside to making inst have names and pulse not have names, but I'm interested in hearing others' ideas.

@lcapelluto
Copy link

So excited for this! It will be a grand improvement to the Pulse API! ❤️

Copy link

@lcapelluto lcapelluto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving bc the substance is all 👍👍
Remaining comments are: names field, and the import path deprecation, since we will want to rename commands/

@lcapelluto
Copy link

To reviewers: Plan right now is to start this on 2/24, so please make comments before then :)

@itoko
Copy link
Contributor

itoko commented Feb 13, 2020

I agree with this refactoring. It will make the API more user-friendly while minimizing the negative impact on performance. Removing commands (except for Pulses) for simpler API makes sense to me since the performance gain by reusing their objects was very limited.

Regarding names, which @lcapelluto raised as an issue, I have another opinion, Pulses should have names too, i.e. unfortunately we have no clean solution for names, either of them have to be overwritten (Instruction.auto_name < Pulse.name < Instruction.user_name?).

If thinking only about visualization, I think Pulses should have names. People would like to name their Pulse when they create it, and, when drawing, they'd like to know which pulse shape they're seeing, e.g. pi_pulse or my_pi_pulse (those would have very similar shape, so hard to distinguish them by shapes). I guess this cannot happen to Instructions other than Play.

I think the reason why the current Instructions have names is for regarding single Instruction as Schedule (not for visualization). (Note: Schedule need to have a name so that users can retrieve the result by the name.) That means to allow execute(inst) and sched = inst_1 + inst_2. If we were able to deprecate those APIs and allow only execute(sched) and sched = Schedule() + inst_1 + inst_2, we would not need to have names in Instructions. (Or if we were able to deprecate the retrieve-by-name API, we'd not need any names for Instruction/Schedule/QuantumCircuit.) However, these kind of further API changes seems too much just for removing names, and also they are beyond the scope of refactoring proposed in this RFC, so should be discussed separately. Assume to keep the other APIs, we need to keep names in Instructions.

@taalexander
Copy link
Collaborator Author

Regarding names, which @lcapelluto raised as an issue, I have another opinion, Pulses should have names too, i.e. unfortunately we have no clean solution for names, either of them have to be overwritten (Instruction.auto_name < Pulse.name < Instruction.user_name?).

We can allow the pulse to have a name.

@lcapelluto
Copy link

Start: Qiskit/qiskit#3862

@eggerdj
Copy link
Contributor

eggerdj commented Feb 19, 2020

I like this RFC. It will help make the pulse API easier to use. I also agree that pulses should have names. This will make it easier for users to keep track of their pulses.

Judging from the RFC it looks like Play appends pulses to channels. Has there been any though on a more sophisticated Play instruction? I.e. one that would let you define the time at which a pulse should be played? Or that lets you insert a pulse between other pulses? How easy is it to replace a pulse? For instance, suppose Alice gives Bob a schedule with some pulses. There is a placeholder Delay in between some pulses. How easy would it be for Bob to replace the Delay with a custom pulse he has?

@nkanazawa1989
Copy link
Contributor

The RFC looks good. I have few comments:

(1) Since aquire is now defined for each channel as well as other instructions, I prefer property name ‘channel’ rather than array like ‘channels’. This benefits to remove overhead of syntax verification because we cannot insert multiple channels to Play for example. I also prefer to keep memory and register slot to be separated to avoid multiple type checking in assembler.

(2) Can we integrate insert operation? E.g. decorator taking instruction and t0 so as to ‘Play(pulse, d0, 100)’ for example.

@itoko
Copy link
Contributor

itoko commented Feb 20, 2020

@nkanazawa1989, for (2), I think no need for it, you can do `Play(pulse, d0) << 100’ instead.

@taalexander
Copy link
Collaborator Author

taalexander commented Feb 20, 2020

I like this RFC. It will help make the pulse API easier to use. I also agree that pulses should have names. This will make it easier for users to keep track of their pulses.

Judging from the RFC it looks like Play appends pulses to channels. Has there been any though on a more sophisticated Play instruction? I.e. one that would let you define the time at which a pulse should be played? Or that lets you insert a pulse between other pulses? How easy is it to replace a pulse? For instance, suppose Alice gives Bob a schedule with some pulses. There is a placeholder Delay in between some pulses. How easy would it be for Bob to replace the Delay with a custom pulse he has?

This will be accomplished with the builder interface we will be defined shortly. I expect we will define a canonical Schedule (what currently exists) and then extend the instructions with scheduling directives that will be removed in a rescheduling canonicalization pass. The other option is to produce Schedules that have these scheduled properties and then combine them in the builder as we did for our hackathon implementation.

@taalexander
Copy link
Collaborator Author

taalexander commented Feb 20, 2020

The RFC looks good. I have few comments:

(1) Since aquire is now defined for each channel as well as other instructions, I prefer property name ‘channel’ rather than array like ‘channels’. This benefits to remove overhead of syntax verification because we cannot insert multiple channels to Play for example. I also prefer to keep memory and register slot to be separated to avoid multiple type checking in assembler.

Channels is a property for every Instruction so that we may resolve dependencies.

(2) Can we integrate insert operation? E.g. decorator taking instruction and t0 so as to ‘Play(pulse, d0, 100)’ for example.

No this would not be an instruction then. We will incorporate such behavior in the builder interface.

@nkanazawa1989
Copy link
Contributor

Is there any instruction taking multiple channels?

@lcapelluto
Copy link

@nkanazawa1989 only sort of. MemorySlots are technically channels, but even Acquire instructions are being migrated to only accept one AcquireChannel and one MemorySlot -- which is still only one physical channel.

@lcapelluto
Copy link

lcapelluto commented Feb 20, 2020

Some of the recent discussion has been regarding timing rather than Instruction/Command unification. I'm calling out of scope on that, especially in light of this: Qiskit/qiskit#3749

When we have relatively timed instructions, most of the considerations you all have brought up above change slightly or dramatically. E.g. accepting t0 will not be possible when we are trying to remove the association between instructions and their start times.

@nkanazawa1989 @itoko @eggerdj ^

@nkanazawa1989
Copy link
Contributor

@nkanazawa1989 only sort of. MemorySlots are technically channels, but even Acquire instructions are being migrated to only accept one AcquireChannel and one MemorySlot -- which is still only one physical channel.

@lcapelluto if we can say they are one physical channel, the property name should be channel. The singular form explicitly presents the design of instructions - single instruction is uniquely mapped to one physical channel.

@taalexander
Copy link
Collaborator Author

taalexander commented Feb 26, 2020

@lcapelluto if we can say they are one physical channel, the property name should be channel. The singular form explicitly presents the design of instructions - single instruction is uniquely mapped to one physical channel.

We cannot say this. We may have synchronization directives such as a barrier that may take multiple channels. I am not comfortable restricting us to this assumption at this time. We should revisit in the future. If one truly requires a singular channel for an instruction they can implement this method on the instruction.

@taalexander taalexander merged commit ace40ab into Qiskit:master Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants