Repository: Qiskit/qiskit-terra (python)

Issue:
unexpected behaviour of pad/MemorySlot has same prefix as MeasureChannel
<!-- ⚠️ If you do not respect this template, your issue will be closed -->
<!-- ⚠️ Make sure to browse the opened and closed issues -->

### 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.




Pull request:
Fixes #7078: Change prefix of MemorySlot channel in Qasm Instructions
<!--
⚠️ If you do not respect this template, your pull request will be closed.
⚠️ Your pull request title should be short detailed and understandable for all.
⚠️ Also, please add a release note file using reno if the change needs to be
  documented in the release notes.
⚠️ If your pull request fixes an open issue, please link to the issue.

- [x] I have added the tests to cover my changes.
- [ ] I have updated the documentation accordingly. (no documentation required as this is a bug fix.)
- [x] I have read the CONTRIBUTING document.
-->

### Summary

Fixes #7078.

* Add new ClassicalIOChannel class as a subclass of pulse.Channel and a superclass of MemorySlot, RegisterSlot and SnapshotChannel.
* In qiskit.pulse.transforms.pad, do not apply a delay to instances of ClassicalIOChannels.
* In the constructors of ShiftFrequency, SetFrequency, SetPhase and ShiftPhase in pulse.instructions, enforce the existing type restriction of the `channel` parameter to PulseChannel by raising a PulseError if the channel is not a PulseChannel.


### Details and comments
This fixes a bug whereby MeasureSlots and MemorySlots are indistinguishable after being serialized into Qobj instructions because they both use the same prefix. 

Documentation is necessary due to the change in behaviour of pad and the introduction of the new type. The release note remains to be written.


Patch:
diff --git a/qiskit/pulse/channels.py b/qiskit/pulse/channels.py
index 175d93ef8..c88fddadd 100644
--- a/qiskit/pulse/channels.py
+++ b/qiskit/pulse/channels.py
@@ -165,6 +165,12 @@ class PulseChannel(Channel, metaclass=ABCMeta):
     pass
 
 
+class ClassicalIOChannel(Channel, metaclass=ABCMeta):
+    """Base class of classical IO channels. These cannot have instructions scheduled on them."""
+
+    pass
+
+
 class DriveChannel(PulseChannel):
     """Drive channels transmit signals to qubits which enact gate operations."""
 
@@ -192,7 +198,7 @@ class AcquireChannel(Channel):
     prefix = "a"
 
 
-class SnapshotChannel(Channel):
+class SnapshotChannel(ClassicalIOChannel):
     """Snapshot channels are used to specify instructions for simulators."""
 
     prefix = "s"
@@ -202,13 +208,13 @@ class SnapshotChannel(Channel):
         super().__init__(0)
 
 
-class MemorySlot(Channel):
+class MemorySlot(ClassicalIOChannel):
     """Memory slot channels represent classical memory storage."""
 
     prefix = "m"
 
 
-class RegisterSlot(Channel):
+class RegisterSlot(ClassicalIOChannel):
     """Classical resister slot channels represent classical registers (low-latency classical
     memory).
     """
diff --git a/qiskit/pulse/instructions/frequency.py b/qiskit/pulse/instructions/frequency.py
index 09233080b..15866e0ae 100644
--- a/qiskit/pulse/instructions/frequency.py
+++ b/qiskit/pulse/instructions/frequency.py
@@ -16,7 +16,7 @@ the frequency of a channel.
 from typing import Optional, Union, Tuple
 
 from qiskit.circuit.parameterexpression import ParameterExpression
-from qiskit.pulse.channels import PulseChannel
+from qiskit.pulse.channels import PulseChannel, PulseError
 from qiskit.pulse.instructions.instruction import Instruction
 
 
@@ -46,7 +46,13 @@ class SetFrequency(Instruction):
             frequency: New frequency of the channel in Hz.
             channel: The channel this instruction operates on.
             name: Name of this set channel frequency instruction.
+        Raises:
+            PulseError: If channel is not a PulseChannel.
         """
+        if not isinstance(channel, PulseChannel):
+            raise PulseError(
+                "The `channel` argument to `SetFrequency` must be of type `channels.PulseChannel`."
+            )
         if not isinstance(frequency, ParameterExpression):
             frequency = float(frequency)
         super().__init__(operands=(frequency, channel), name=name)
@@ -93,9 +99,15 @@ class ShiftFrequency(Instruction):
             frequency: Frequency shift of the channel in Hz.
             channel: The channel this instruction operates on.
             name: Name of this set channel frequency instruction.
+        Raises:
+            PulseError: If channel is not a PulseChannel.
         """
         if not isinstance(frequency, ParameterExpression):
             frequency = float(frequency)
+        if not isinstance(channel, PulseChannel):
+            raise PulseError(
+                "The `channel` argument to `ShiftFrequency` must be of type `channels.PulseChannel`."
+            )
         super().__init__(operands=(frequency, channel), name=name)
 
     @property
diff --git a/qiskit/pulse/instructions/phase.py b/qiskit/pulse/instructions/phase.py
index b9067486d..d8db037a5 100644
--- a/qiskit/pulse/instructions/phase.py
+++ b/qiskit/pulse/instructions/phase.py
@@ -18,7 +18,7 @@ relative amount.
 from typing import Optional, Union, Tuple
 
 from qiskit.circuit import ParameterExpression
-from qiskit.pulse.channels import PulseChannel
+from qiskit.pulse.channels import PulseChannel, PulseError
 from qiskit.pulse.instructions.instruction import Instruction
 
 
@@ -52,7 +52,14 @@ class ShiftPhase(Instruction):
             phase: The rotation angle in radians.
             channel: The channel this instruction operates on.
             name: Display name for this instruction.
+
+        Raises:
+            PulseError: If channel is not a PulseChannel.
         """
+        if not isinstance(channel, PulseChannel):
+            raise PulseError(
+                "The `channel` argument to `ShiftPhase` must be of type `channels.PulseChannel`."
+            )
         super().__init__(operands=(phase, channel), name=name)
 
     @property
@@ -108,7 +115,13 @@ class SetPhase(Instruction):
             phase: The rotation angle in radians.
             channel: The channel this instruction operates on.
             name: Display name for this instruction.
+        Raises:
+            PulseError: If channel is not a PulseChannel.
         """
+        if not isinstance(channel, PulseChannel):
+            raise PulseError(
+                "The `channel` argument to `SetPhase` must be of type `channels.PulseChannel`."
+            )
         super().__init__(operands=(phase, channel), name=name)
 
     @property
diff --git a/qiskit/pulse/instructions/play.py b/qiskit/pulse/instructions/play.py
index 2a8536643..41effc205 100644
--- a/qiskit/pulse/instructions/play.py
+++ b/qiskit/pulse/instructions/play.py
@@ -41,7 +41,7 @@ class Play(Instruction):
             name: Name of the instruction for display purposes. Defaults to ``pulse.name``.
 
         Raises:
-            PulseError: If pulse is not a Pulse type.
+            PulseError: If pulse is not a Pulse type, or channel is not a PulseChannel.
         """
         if not isinstance(pulse, Pulse):
             raise PulseError("The `pulse` argument to `Play` must be of type `library.Pulse`.")
diff --git a/qiskit/pulse/transforms/canonicalization.py b/qiskit/pulse/transforms/canonicalization.py
index a019de2ed..4e2409ffa 100644
--- a/qiskit/pulse/transforms/canonicalization.py
+++ b/qiskit/pulse/transforms/canonicalization.py
@@ -18,6 +18,7 @@ from typing import List, Optional, Iterable, Union
 import numpy as np
 
 from qiskit.pulse import channels as chans, exceptions, instructions
+from qiskit.pulse.channels import ClassicalIOChannel
 from qiskit.pulse.exceptions import PulseError
 from qiskit.pulse.exceptions import UnassignedDurationError
 from qiskit.pulse.instruction_schedule_map import InstructionScheduleMap
@@ -475,6 +476,8 @@ def pad(
     channels = channels or schedule.channels
 
     for channel in channels:
+        if isinstance(channel, ClassicalIOChannel):
+            continue
         if channel not in schedule.channels:
             schedule |= instructions.Delay(until, channel)
             continue
diff --git a/releasenotes/notes/introduce-classical-io-channel-0a616e6ca75b7687.yaml b/releasenotes/notes/introduce-classical-io-channel-0a616e6ca75b7687.yaml
new file mode 100644
index 000000000..71a79556a
--- /dev/null
+++ b/releasenotes/notes/introduce-classical-io-channel-0a616e6ca75b7687.yaml
@@ -0,0 +1,13 @@
+---
+features:
+  - |
+    In the qiskit.pulse.channels package, ClassicalIOChannel class has been added
+    as an abstract base class of MemorySlot, RegisterSlot, and SnapshotChannel.
+
+    The qiskit.pulse.transforms.canonicalization.pad method does not introduce
+    delays to any channels which are instances of ClassicalIOChannel.
+
+    In qiskit.pulse.instructions, the constructors to SetPhase, ShiftPhase,
+    SetFrequency and ShiftFrequency now throw a PulseError if the channel parameter
+    is not of type PulseChannel.
+


Test patch:
diff --git a/test/python/pulse/test_channels.py b/test/python/pulse/test_channels.py
index 3637a1cea..604202fb5 100644
--- a/test/python/pulse/test_channels.py
+++ b/test/python/pulse/test_channels.py
@@ -17,8 +17,9 @@ import unittest
 from qiskit.pulse.channels import (
     AcquireChannel,
     Channel,
-    DriveChannel,
+    ClassicalIOChannel,
     ControlChannel,
+    DriveChannel,
     MeasureChannel,
     MemorySlot,
     PulseChannel,
@@ -67,8 +68,17 @@ class TestAcquireChannel(QiskitTestCase):
         self.assertEqual(hash_1, hash_2)
 
 
+class TestClassicalIOChannel(QiskitTestCase):
+    """Test base classical IO channel."""
+
+    def test_cannot_be_instantiated(self):
+        """Test base classical IO channel cannot be instantiated."""
+        with self.assertRaises(NotImplementedError):
+            ClassicalIOChannel(0)
+
+
 class TestMemorySlot(QiskitTestCase):
-    """AcquireChannel tests."""
+    """MemorySlot tests."""
 
     def test_default(self):
         """Test default memory slot."""
@@ -76,6 +86,7 @@ class TestMemorySlot(QiskitTestCase):
 
         self.assertEqual(memory_slot.index, 123)
         self.assertEqual(memory_slot.name, "m123")
+        self.assertTrue(isinstance(memory_slot, ClassicalIOChannel))
 
 
 class TestRegisterSlot(QiskitTestCase):
@@ -87,6 +98,7 @@ class TestRegisterSlot(QiskitTestCase):
 
         self.assertEqual(register_slot.index, 123)
         self.assertEqual(register_slot.name, "c123")
+        self.assertTrue(isinstance(register_slot, ClassicalIOChannel))
 
 
 class TestSnapshotChannel(QiskitTestCase):
@@ -98,6 +110,7 @@ class TestSnapshotChannel(QiskitTestCase):
 
         self.assertEqual(snapshot_channel.index, 0)
         self.assertEqual(snapshot_channel.name, "s0")
+        self.assertTrue(isinstance(snapshot_channel, ClassicalIOChannel))
 
 
 class TestDriveChannel(QiskitTestCase):
diff --git a/test/python/pulse/test_instructions.py b/test/python/pulse/test_instructions.py
index a5284c27e..111ebd35f 100644
--- a/test/python/pulse/test_instructions.py
+++ b/test/python/pulse/test_instructions.py
@@ -156,6 +156,64 @@ class TestSetFrequency(QiskitTestCase):
         )
         self.assertEqual(repr(set_freq), "SetFrequency(4500000000.0, DriveChannel(1), name='test')")
 
+    def test_freq_non_pulse_channel(self):
+        """Test set frequency constructor with illegal channel"""
+        with self.assertRaises(exceptions.PulseError):
+            instructions.SetFrequency(4.5e9, channels.RegisterSlot(1), name="test")
+
+
+class TestShiftFrequency(QiskitTestCase):
+    """Shift frequency tests."""
+
+    def test_shift_freq(self):
+        """Test shift frequency basic functionality."""
+        shift_freq = instructions.ShiftFrequency(4.5e9, channels.DriveChannel(1), name="test")
+
+        self.assertIsInstance(shift_freq.id, int)
+        self.assertEqual(shift_freq.duration, 0)
+        self.assertEqual(shift_freq.frequency, 4.5e9)
+        self.assertEqual(shift_freq.operands, (4.5e9, channels.DriveChannel(1)))
+        self.assertEqual(
+            shift_freq, instructions.ShiftFrequency(4.5e9, channels.DriveChannel(1), name="test")
+        )
+        self.assertNotEqual(
+            shift_freq, instructions.ShiftFrequency(4.5e8, channels.DriveChannel(1), name="test")
+        )
+        self.assertEqual(
+            repr(shift_freq), "ShiftFrequency(4500000000.0, DriveChannel(1), name='test')"
+        )
+
+    def test_freq_non_pulse_channel(self):
+        """Test shift frequency constructor with illegal channel"""
+        with self.assertRaises(exceptions.PulseError):
+            instructions.ShiftFrequency(4.5e9, channels.RegisterSlot(1), name="test")
+
+
+class TestSetPhase(QiskitTestCase):
+    """Test the instruction construction."""
+
+    def test_default(self):
+        """Test basic SetPhase."""
+        set_phase = instructions.SetPhase(1.57, channels.DriveChannel(0))
+
+        self.assertIsInstance(set_phase.id, int)
+        self.assertEqual(set_phase.name, None)
+        self.assertEqual(set_phase.duration, 0)
+        self.assertEqual(set_phase.phase, 1.57)
+        self.assertEqual(set_phase.operands, (1.57, channels.DriveChannel(0)))
+        self.assertEqual(
+            set_phase, instructions.SetPhase(1.57, channels.DriveChannel(0), name="test")
+        )
+        self.assertNotEqual(
+            set_phase, instructions.SetPhase(1.57j, channels.DriveChannel(0), name="test")
+        )
+        self.assertEqual(repr(set_phase), "SetPhase(1.57, DriveChannel(0))")
+
+    def test_set_phase_non_pulse_channel(self):
+        """Test shift phase constructor with illegal channel"""
+        with self.assertRaises(exceptions.PulseError):
+            instructions.SetPhase(1.57, channels.RegisterSlot(1), name="test")
+
 
 class TestShiftPhase(QiskitTestCase):
     """Test the instruction construction."""
@@ -177,6 +235,11 @@ class TestShiftPhase(QiskitTestCase):
         )
         self.assertEqual(repr(shift_phase), "ShiftPhase(1.57, DriveChannel(0))")
 
+    def test_shift_phase_non_pulse_channel(self):
+        """Test shift phase constructor with illegal channel"""
+        with self.assertRaises(exceptions.PulseError):
+            instructions.ShiftPhase(1.57, channels.RegisterSlot(1), name="test")
+
 
 class TestSnapshot(QiskitTestCase):
     """Snapshot tests."""
diff --git a/test/python/pulse/test_transforms.py b/test/python/pulse/test_transforms.py
index 49038afdb..e19d024c2 100644
--- a/test/python/pulse/test_transforms.py
+++ b/test/python/pulse/test_transforms.py
@@ -29,7 +29,13 @@ from qiskit.pulse import (
     Constant,
 )
 from qiskit.pulse import transforms, instructions
-from qiskit.pulse.channels import MemorySlot, DriveChannel, AcquireChannel
+from qiskit.pulse.channels import (
+    MemorySlot,
+    DriveChannel,
+    AcquireChannel,
+    RegisterSlot,
+    SnapshotChannel,
+)
 from qiskit.pulse.instructions import directives
 from qiskit.test import QiskitTestCase
 from qiskit.providers.fake_provider import FakeOpenPulse2Q
@@ -349,6 +355,23 @@ class TestPad(QiskitTestCase):
 
         self.assertEqual(transforms.pad(sched, until=30, inplace=True), ref_sched)
 
+    def test_pad_no_delay_on_classical_io_channels(self):
+        """Test padding does not apply to classical IO channels."""
+        delay = 10
+        sched = (
+            Delay(delay, MemorySlot(0)).shift(20)
+            + Delay(delay, RegisterSlot(0)).shift(10)
+            + Delay(delay, SnapshotChannel())
+        )
+
+        ref_sched = (
+            Delay(delay, MemorySlot(0)).shift(20)
+            + Delay(delay, RegisterSlot(0)).shift(10)
+            + Delay(delay, SnapshotChannel())
+        )
+
+        self.assertEqual(transforms.pad(sched, until=15), ref_sched)
+
 
 def get_pulse_ids(schedules: List[Schedule]) -> Set[int]:
     """Returns ids of pulses used in Schedules."""