unexpected behaviour of pad/MemorySlot has same prefix as MeasureChannel

[stuckmar]

Information

• Qiskit Terra version:
• 18.2
• Python version:
• 3.8.11
• Operating system:
• Windows 10 Home 20H2

What is the current behavior?

Starting with a Schedule containing an Acquire, with the Acquire using a MemorySlot, and any Delay or Pulse, which is longer than that of the Acquire. Now using the pad function on this Schedule will return a Schedule, which contains Delays on both the AcquireChannel and MemorySlot. When the padded schedule is now assembled and converted, using the QobjToInstructionConverter and again a new schedule is created from the converted instructions, the new Schedule does not contain a Delay on the AcquireChannel, nor on the MemorySlot, but a Delay on the MeasureChannel with the same index as the MemorySlot used.

Steps to reproduce the problem

from qiskit.pulse import Schedule, Play, Acquire, MemorySlot, AcquireChannel, DriveChannel
from qiskit.pulse.library.parametric_pulses import Constant
from qiskit.pulse.transforms import pad
from qiskit.qobj.converters.pulse_instruction import QobjToInstructionConverter
from qiskit import assemble

sched=Schedule()
sched+=Play(Constant(4, 0.1), DriveChannel(0))
sched+=Acquire(3, AcquireChannel(0), MemorySlot(0))
sched=pad(sched, inplace=False)

qobj=assemble(sched, qubit_lo_freq=[4.5e9], meas_lo_freq=[5e9], parametric_pulses=['constant'])

converter = QobjToInstructionConverter([])
sched2=Schedule(*[converter(inst) for inst in qobj.experiments[0].instructions])

Now sched and sched2 contain different instructions:

sched.instructions

((0, Acquire(3, AcquireChannel(0), MemorySlot(0))),
(0, Play(Constant(duration=4, amp=(0.1+0j)), DriveChannel(0))),
(3, Delay(1, AcquireChannel(0))),
(3, Delay(1, MemorySlot(0))))

sched2.instructions

((0, Acquire(3, AcquireChannel(0), MemorySlot(0))),
(0,
Play(Constant(duration=4, amp=(0.1+0j), name='constant_e3a2'), DriveChannel(0), name='constant_e3a2')),
(3, Delay(1, MeasureChannel(0))))

What is the expected behavior?

Assembling, converting, and creating a Schedule should return the same Schedule, which was assembled.

Suggested solutions

First, I am not sure if it is expected from the pad function to create Delays on AcquireChannels and MemorySlots. Since both of them are not PulseChannels and the assembled Qobj does not contain the Delay on the AcquireChannel.

Second, the prefix of the MemorySlot is 'm', which is the same as for MeasureChannel. Therefore, the conversion of these channels happen. I suggest a different prefix for MemorySlot, for example 's'. Then in qiskit\qobj\converters\pulse_instruction.py the get_channel method of QobjToInstructionConverter needs to be expanded to include MemorySlots, if Delays make sense on MemorySlots. Alternatively, a Delay on a MemorySlot could be skipped entirely by the assembler, similiar how it is done with Delays on AcquireChannels.

[javabster]

hi @stuckmar thanks for raising this issue, it is currently unclear if AcquireChannels should allow Delay instructions and there are pros and cons to allowing it. If you would like to submit a PR with your suggested solution and some more justification for your solution choice please go ahead 😄 Otherwise the core team will look into it when they have capacity.

@lcapelluto may have some additional thoughts too

[Osmanilge]

Hello I want to assigned for this issue. I am new in qiskit but I am interested in Qiskit so may you inform me about issue? I hope this issue is suitable for me.

[1ucian0]

Hi @Osmanilge I think the best way to assess If the issue is suitable for you, have a look to the suggested solution. If it sounds like greek, check a different one less obscure. Otherwise, let me know and I can assign it to you!

[HuangJunye]

Hey @Osmanilge, are you still working on this issue? Let us know if you need any help 😃 If you’re not working on this anymore let us know and we will un-assign you so others can pick it up.

[Osmanilge]

this anymor

I am really sorry. I could not realize I am assigned for this issue. I will look this issue if you want me to still assigned otherwise if it needs to finished quickly , you can un-assign me.

[jakelishman]

Don't worry about it - it's not urgent, and you were never formally assigned. If something becomes urgent for us, we can solve things in-house as well.

If you'd like to work on the issue and have time to do so, let us know, otherwise no worries.

[pollyshaw]

Please could I be assigned to this? I have put in a PR.

[HuangJunye]

@pollyshaw Sure, I have assigned you. Thanks for making the contribution!

[taalexander]

Please see my comment here.

[wshanks]

@taalexander @nkanazawa1989 Isn't the problem really that here:

>>> Acquire(3, AcquireChannel(0), MemorySlot(0)).channels
(AcquireChannel(0), MemorySlot(0))

MemorySlot(0) is being reported as a channel even though it is not one? Do you think anything is using the fact that the MemorySlot is getting reported in the acquire channels or can Acquire.channels be changed to only report the acquire channel?

[nkanazawa1989]

Oh, you're right! This should appear in Qiskit programs and thus memory slot and register slot are actually scheduled. Probably we need to update this method to return only acquire channel? Since these slots are always sync with the acquire channel and thus even not visualized. I don't think removing them breaks code execution.
https://github.com/Qiskit/qiskit-terra/blob/8061cfe04e33cc2827120b3aa1e2ddabcab38c6f/qiskit/pulse/instructions/acquire.py#L90-L93

Anyways, in Qobj converter, each instruction kind is separately handled and the memory slots are generated in a part of acquire serialization. So missing timeslot for memory slot should be fine.
https://github.com/Qiskit/qiskit-terra/blob/8061cfe04e33cc2827120b3aa1e2ddabcab38c6f/qiskit/qobj/converters/pulse_instruction.py#L502

[pollyshaw]

OK it sounds like I should revert these changes and instead change the implementation of Acquire.channels(). Will do so tomorrow.

[pollyshaw]

@nkanazawa1989 Thanks for approving this! Is there anything else I need to do now? I don't seem to have a 'merge' button so perhaps a contributor needs to merge it.

[nkanazawa1989]

I'll merge if other people are happy with the changes (or don't plan to review). @wshanks @taalexander ?

[nkanazawa1989]

congrats @pollyshaw ! Thanks for your contribution :)