Update pulse instruction interface to the new pulse model #11726
Conversation
|
One or more of the the following people are requested to review this:
|
nkanazawa1989
added
experimental
There was a problem hiding this comment.
Thanks @TsafrirA for your hard work of unittest update. I like new pattern for instruction construction. Since new model allows multiple patterns to specify how the instruction is executed, user must explicitly give operands. Although new pattern looks complicated, I believe our target users are familiar with the concept and they can use new model with well-written migration guide (TODO).
On the other hand, tons of changes to the unittests is an indication of breaking API change. In some minor release of 1.0 we should add deprecation warning for the use of non-keyword args. This is just a reminder and doesn't block this PR.
qiskit/pulse/instructions/acquire.py
Outdated
| @property | ||
| def channel(self) -> AcquireChannel: | ||
| def channel(self) -> Union[AcquireChannel, None]: |
There was a problem hiding this comment.
Can we deprecate channel and acquire at minimum cost?
There was a problem hiding this comment.
Once we're done, I think yes.
| return inst_target | ||
|
|
||
| @property | ||
| def channel(self) -> Union[PulseChannel, None]: |
There was a problem hiding this comment.
Because we will deprecate channel at some point and move to mixed-frame model, we don't need to add channel property to newly introduced classes.
There was a problem hiding this comment.
This isn't really an addition. It's just a way to keep the channel property of all frame instructions with less code clutter. It will make it easier to deprecate it later.
There was a problem hiding this comment.
I would add channel property to each subclass rather in the baseclass. In this way newly added instructions will not gain legacy property. Anyways this is small difference and doesn't block merge.
| ``FrameInstruction`` should be considered abstract, and should not be instantiated as is. | ||
| """ | ||
|
|
||
| def _validate_and_format_frame( |
There was a problem hiding this comment.
Perhaps you also need to overwrite _validate in the sueprclass? Since Instruction._validate is called inside the constructor and it checks if all self.channels are Channel instance. When either mixed_frame or frame is passed, self.channel returns None and the validation will fail.
qiskit/pulse/instructions/play.py
Outdated
| if (target is not None) != (frame is not None): | ||
| raise PulseError("target and frame must be provided simultaneously") | ||
| if (channel is not None) + (mixed_frame is not None) + (target is not None) != 1: | ||
| raise PulseError( | ||
| "Exactly one of mixed_frame, channel or the duo target and frame has to be provided" | ||
| ) | ||
|
|
||
| if target is not None: | ||
| if not isinstance(target, PulseTarget): | ||
| raise PulseError(f"Expected a PulseTarget, got {target} instead.") | ||
| if not isinstance(frame, Frame): | ||
| raise PulseError(f"Expected a Frame, got {frame} instead.") | ||
| mixed_frame = MixedFrame(target, frame) | ||
| else: | ||
| mixed_frame = mixed_frame or channel |
There was a problem hiding this comment.
| if (target is not None) != (frame is not None): | |
| raise PulseError("target and frame must be provided simultaneously") | |
| if (channel is not None) + (mixed_frame is not None) + (target is not None) != 1: | |
| raise PulseError( | |
| "Exactly one of mixed_frame, channel or the duo target and frame has to be provided" | |
| ) | |
| if target is not None: | |
| if not isinstance(target, PulseTarget): | |
| raise PulseError(f"Expected a PulseTarget, got {target} instead.") | |
| if not isinstance(frame, Frame): | |
| raise PulseError(f"Expected a Frame, got {frame} instead.") | |
| mixed_frame = MixedFrame(target, frame) | |
| else: | |
| mixed_frame = mixed_frame or channel | |
| if (channel is not None) + (mixed_frame is not None) + (target is not None) != 1: | |
| raise PulseError( | |
| "Exactly one of mixed_frame, channel or the duo target and frame has to be provided" | |
| ) | |
| if target is not None: | |
| if not isinstance(target, PulseTarget): | |
| raise PulseError(f"Expected a PulseTarget, got {target} instead.") | |
| if not isinstance(frame, Frame): | |
| raise PulseError(f"Expected a Frame, got {frame} instead.") | |
| mixed_frame = MixedFrame(target, frame) | |
| else: | |
| mixed_frame = mixed_frame or channel |
Also equivalent right?
There was a problem hiding this comment.
This will actually fail to raise an error if you provide frame and mixed_frame (the frame argument will be ignored).
| """ | ||
| for key in self.ref_keys: | ||
| ref_keys = (name,) + tuple(extra_keys) |
There was a problem hiding this comment.
Why did you move validation to the constructor? _validate is a base class method which is called after class is instantiated, which defines an extra validation protocol for instruction-specific operands. This makes constructor bit cleaner. If you don't like it, feel free to remove it from the base class (because this is just a protected method). Since you added lots of typecheck directly in the constructor, this delayed validation mechanism doesn't fit in with new framework.
nkanazawa1989
changed the title
|
Thanks @nkanazawa1989 for the quick review. I've modified the tests and some of the validation logic as you requested. As I wrote, I removed entirely the |
|
@nkanazawa1989 |
Summary
The Pulse Instructions interface is updated to accommodate the new pulse model (#10694).
Details and comments
Following discussions in #11464 (which is replaced by this PR), it was decided that the new pulse model will be introduced into the instructions interface in the following way.
The signature of instruction initialization will be updated to (demonstrated here for the
Playinstruction, but the same logic holds for all relevant instructions):Upon initialization, the input arguments are validated to guarantee that a correct combination is provided (mixed frame for Play, mixed frame\frame for phase\frequency instructions, etc.). This signature is the most flexible one, while also providing a path for a minimally breaking transition from the old model to the new model. An intermediate signature during the deprecation period (Qiskit 1.x) would be:
such that a minimal change is needed to update existing code.
As discussed in #11464, this option is not without limitations, and it is quite possible that eventually the interface would change. However, with its flexibility the option presented here provides a solid starting step, which could be easily modified later on.
For posterity, it should be mentioned that another option which was favorably considered is:
The presented implementation removes the
_validateabstract method. The method was implemented individually for all instructions, and thus didn't reduce code, and hid the errors away from the initialization function. However, aFrameInstructionsubclass was introduced with a_validatefunction, because the validation logic is shared across all set\shift phase\frequency instructions.The following instructions were not changed:
The
Acquireinstruction was updated in a shallow way, and requires further discussion. Main questions include -MemorySlot(RegisterSlot) class sit in the new model?Qubit?RegisterSlot? Does it serve any function on any relevant backend?