-
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
Qiskit Pulse - Introduce PulseIR #11021
Conversation
…iskit-terra into LogicalElementsFrames
…iskit-terra into LogicalElementsFrames
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Naoki Kanazawa <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
Co-authored-by: Will Shanks <[email protected]>
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 6637721141
💛 - Coveralls |
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.
Some initial comments are included in this review.
One component that is unclear to me is what is the underlying hardware resources? Ie see that we can have Qubit
s and Coupler
s which define/connect qubits and presumably are helpful for lowering to the underlying hardware resources.
However, frames accept these. My question is what does the resolution of the underlying hardware resource tie to which is ultimately need to connect to an instrument?
It seems we would need a Port/Channel
object to which the frames would tie to. Giving us:
- Qubit/coupler
- frame
- port/channel on hardware (resolved internally)
Otherwise, I wonder what might happen for say experiments with multiple defined user frames and resolving them to hardware?
@@ -0,0 +1,335 @@ | |||
# This code is part of Qiskit. |
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 are already in the namespace and likely should be just ir/instructions.py
?
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.
There are already several Instruction
types in Qiskit, one of them in Pulse. On the one hand it might be more confusing, on the other hand, we already resolved all of the ambiguous referencing issues. @nkanazawa1989 , what do you think?
""" | ||
|
||
@abstractmethod | ||
def __init__(self, duration: int, operand, initial_time: Optional[int] = None): |
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.
In the long run you might want to consider typing duration
other than int
to enable reasoning about pulses of say different units and providing automated transformation routines at compile time.
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.
Have you thought about separating the concerns of the instruction and its timing information within a larger schedule? Having a higher-level structure that encapsulated the notion of time outside (or via attribution) to the instructions which would lead to better separation of concerns?
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.
Because scheduling is such a major part of the compiler's job, I think it's not very productive to separate this issue. What is the benefit you see in separating this?
* Play | ||
* Delay | ||
* SetFrequency | ||
* ShiftFrequency | ||
* SetPhase | ||
* ShiftPhase | ||
|
||
Instructions are defined on both ``LogicalElement`` and ``Frame`` or in some cases only | ||
one of the two. If only one of ``LogicalElement`` and ``Frame`` are provided, the instruction | ||
is called partial instruction, and will require broadcasting during compilation. | ||
""" | ||
|
||
allowed_types = ["Play", "Delay", "SetFrequency", "ShiftFrequency", "SetPhase", "ShiftPhase"] |
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.
Defining these types within the base type leads to a cyclical relationship and weak typing. It would be better to use subclassing within common values for each class passed to the initializer, ie.,
class NewType
def __init__(
self,
"new_type",
...
):
specifying allowed_types
is arbitrarily constraining.
if instruction_type not in self.__class__.allowed_types: | ||
raise PulseError(f"{instruction_type} is not a recognized instruction") | ||
|
||
if instruction_type == "Delay": | ||
if not isinstance(operand, (int, np.integer)) or operand < 0: | ||
raise PulseError( | ||
"The operand of a Delay instruction must be a non-negative integer." | ||
) | ||
if logical_element is None: | ||
raise PulseError("Delay instruction must have an associated logical element.") | ||
duration = operand | ||
|
||
elif instruction_type == "Play": | ||
if not isinstance(operand, (SymbolicPulse, Waveform)): | ||
raise PulseError( | ||
f"Play instruction is incompatible with operand of type {type(operand)}." | ||
) | ||
if logical_element is None or frame is None: | ||
raise PulseError( | ||
"Play instruction must have an associated logical element and frame." | ||
) | ||
duration = operand.duration | ||
|
||
elif instruction_type in ["SetFrequency", "ShiftFrequency", "SetPhase", "ShiftPhase"]: | ||
if isinstance(operand, (int, np.integer)): | ||
operand = float(operand) | ||
if not isinstance(operand, float): | ||
raise PulseError( | ||
"The operand of a Set/Shift Frequency/Phase instruction must be a float." | ||
) | ||
if frame is None: | ||
raise PulseError( | ||
"Set/Shift Frequency/Phase instruction must have an associated frame." | ||
) | ||
duration = 0 | ||
|
||
return duration |
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.
This logic should be handled by subclassing and not baked into the core class. The need for it is a codesmell that this individual class knows too much.
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.
Thanks. I'll change that.
def __init__( | ||
self, | ||
qubit: Qubit, | ||
memory_slot: MemorySlot, | ||
duration: int, | ||
initial_time: Optional[int] = None, | ||
): |
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.
Why are acquires on qubits? This seems to be mixing representations, ie., at the pulse level there should be no knowledge of qubits since this is what defines a qubit.
class PulseIR: | ||
""" | ||
``PulseIR`` is the backbone of the intermediate representation used in the Qiskit Pulse compiler. | ||
A pulse program is represented as a single ``PulseIR`` object, with elements | ||
which include IR instructions (base class :class:`~.BaseIRInstruction) and other | ||
nested ``PulseIR`` objects. This structure mimics that of :class:`.qiskit.pulse.ScheduleBlock` | ||
which is the main pulse program format used in Qiskit. | ||
|
||
The IR is ment to support compilation needs, including partial instructions broadcasting, | ||
scheduling, validation and more. | ||
""" |
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.
Is this a module, a program, a sequence, a block? I believe the name PulseIR
could be more descriptive.
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.
Thanks. I will change that.
@property | ||
def elements(self) -> List[Union[GenericInstruction, AcquireInstruction, "PulseIR"]]: | ||
"""Return the elements of the ``PulseIR`` object""" | ||
return self._elements |
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.
It seems like there should be a generic type returned that encompasses all of these rather than a list of types.
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.
Thanks. I will change that.
def get_acquire_instructions( | ||
self, qubit: Optional[Qubit] = None, recursive: Optional[bool] = True | ||
) -> List[GenericInstruction]: | ||
"""Return ``AcquireInstruction``\\ s | ||
|
||
Args: | ||
qubit: Optionally return only instructions associated with this qubit. | ||
Default value - None (return all instructions). | ||
recursive: If ``True`` recursively looks for instructions, | ||
else ignores instructions of child ``PulseIR``. Default - ``True``. | ||
""" | ||
instructions = [] | ||
for element in self._elements: | ||
if isinstance(element, AcquireInstruction) and ( | ||
qubit is None or element.qubit == qubit | ||
): | ||
instructions.append(element) | ||
elif recursive and isinstance(element, PulseIR): | ||
instructions.extend(element.get_acquire_instructions(qubit, recursive=recursive)) | ||
return instructions |
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.
It seems like we should provide generic means for getting instructions, not fixed routines for each function?
def get_acquire_instructions( | ||
self, qubit: Optional[Qubit] = None, recursive: Optional[bool] = True |
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.
Same question about why qubits are involved with pulse?
qiskit/pulse/model/__init__.py
Outdated
from .logical_elements import ( | ||
LogicalElement, | ||
Qubit, | ||
Coupler, | ||
) | ||
|
||
from .frames import ( | ||
Frame, | ||
GenericFrame, | ||
QubitFrame, | ||
MeasurementFrame, | ||
) | ||
|
||
from .mixed_frames import ( | ||
MixedFrame, | ||
) |
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.
It seems to me these are all IR components as well and should be tied in a common representation. Ie., within MLIR these would be types generated by operations and seem to be treated as such within frames.py
.
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.
Could you clarify what you mean by "generated by operations"?
Because these objects double as both the IR and the user interface, we wanted to define them separately from the IR.
Superseded by #11767. |
Summary
This PR is the second step in implementing RFC0012. It the introduces the intermediate representation which will be user by the Qiskit Pulse Compiler.
This PR depends on #10694
Details and comments
For more details, see the RFC.
True
andFalse
. I tried to pick the option I thought would be more useful, but the final product might be confusing.Release notes will be added at a later stage for the combined changes.