Add function to obtain the waveform on a PulseChannel #6995
Conversation
snsunx
requested review from
DanPuzzuoli,
eggerdj,
lcapelluto,
nkanazawa1989,
taalexander and
a team
as code owners
|
|
There was a problem hiding this comment.
This is a great start. Could you also please add tests for this functionality? I would also appreciate if @nkanazawa1989 could take a look at this.
There was a problem hiding this comment.
Thanks for taking a stab at waveform transform. I'm grad to see we have another transform function. My two major points are
-
Move frame parser to transform
- Transform should not have dependency on visualizer. You can create new parser in the pulse transform module, then use new transform function in the visualizer.
-
Split function into multiple pieces
- One huge transform function is usually hard to maintain. If we split the transform into multiple atomic functions, we can efficiently write unittest. For example,
- Just call get waveform if any parametric pulse exist (
to_waveform) - Apply phase shift to each waveform (
apply_phase) - Apply carrier waves based on frequency at t0 (
apply_carrier) - Combine all waveforms in the channel to create full waveform array (
to_full_waveform)
- Just call get waveform if any parametric pulse exist (
- By combining these passes with
target_qobj_transform, you can create new transform function, something likewaveform_transform. For the visualizer, you can create another transform function that only performsto_waveformandapply_phase. Note that the visualizer draws pulses individually.
- One huge transform function is usually hard to maintain. If we split the transform into multiple atomic functions, we can efficiently write unittest. For example,
It feels to me that the function should be a method of Schedule rather than a standalone function in a standalone script. I would like to know what the maintainers think.
This is usability vs maintenance issue. I think Schedule is a container of instructions, i.e. a program, and thus the operations (i.e. transforms) and programs should be separated. In Qiskit, QuantumCircuit doesn't take this approach and it has many method to transform itself, however, now the class has nearly 5000 lines of code. The number of lines will be further increased if we add new transform.
This is not preferable in terms of test and code maintenance. If we separate operations from the program, we can implement as many transforms as we want while keeping the size of Schedule class. Any thoughts? @taalexander
|
Thank you two for your comments. I have updated some argument type checking statements, which I think addressed most of @taalexander's comments. One remaining task is to figure out the correct frequency for a I will take a look at how to port the functionalities of |
|
Here you can find some unittests for the pulse transforms. Please let me know if you have any question about the implementation of the Note that according to the OpenPulse specification, control channel can be whatever we can describe as a device Hamiltonian. This means, the frequency computation of this channel may depend on the device architecture. So you need to write a base class that implements abstract method to compute control channel frequencies, along with its subclass that implements a specific computation for IBM devices. |
|
@nkanazawa1989 Thank you! Sorry I have been busy with some other work and haven't been able to get to this. I should be able to give another update later this week. |
|
Hi, sorry for a slightly late update. I moved |
There was a problem hiding this comment.
Thanks @snsunx for the constant update! I think now it is moving toward right direction. You can also create a dedicated folder in the pulse transforms since this transform creates several files. And you need to move some data structure back to drawer, e.g. class LineType(str, Enum):.
I'll convert this PR into draft until the code is finalized :)
| The channel transform manager for the specified channel. | ||
| """ | ||
| # flatten the schedule | ||
| program = target_qobj_transform(program) |
There was a problem hiding this comment.
We can assume this is externally done by writing another transform function including this class like a pass. Otherwise this transform will have unnecessary dependency on target_qobj_transform.
|
Hi @nkanazawa1989, I didn't create a new folder but made the following organizational changes:
I wonder what are your thoughts on these changes. If these changes are ok, I'll go on to write some unit tests and also modify the existing unit tests in Also I made a forced amend push (from |
Yes, that should be okey.
I don't think this is good direction. The |
|
Hi all, I think I finished all the required features. To summarize mainly what I have done
I moved the unit tests around a bit. The new test class The checks were not successful due to two errors from the linter, one being a format issue of the attribute |
There was a problem hiding this comment.
Sorry about slow response. This is really great job and thanks for your hard work!
I think your code is in right direction! However, you don't need to bind yourself to what ChannelEvents originally implemented ;) Indeed the ChannelEvents converts a Schedule into some other program representation that is not well defined (I intended to use this representation only in the drawer). Thus following transforms (if exist) need to write dedicated parser for it.
To avoid that, I would add MicroInstruction (to be used as a data container that can be used in the Schedule, probably you have better name), that would look like
class MicroInstruction(Instruction):
def __init__(
self,
t0: int,
samples: np.ndarray,
channels: Tuple[Channels],
metadata: Optional[Dict] = None,
name: Optional[str] = None,
):
super().__init__(operands=(t0, samples, channels, metadata), name=name)
@property
def duration(self):
return len(self.samples)
@property
def start_time(self):
return self._operands[0]
@property
def stop_time(self):
return self.start_time + self.duration
@property
def samples(self):
return self._operands[1]
@property
def channels(self):
return self._operands[2]
@property
def metadata(self):
return self._operands[3]Then the parsed frequency and phase will be stored as a part of metadata. You can load the instruction metadata and apply phase or sideband modulation to samples in each transfrom pass.
In this approach, we don't need to introduce ParsedInstruction, and most of transform pass function signature will become transform_pass(program: Schedule, *args, **kwargs) -> Schedule which is much more handy. I'd like to hear your thought @taalexander
(EDIT)
regarding the circular import: this is because you are importing pulse module from the pulse module. you should selectively import a specific component as needed basis.
| ydata: np.ndarray = None | ||
|
|
||
|
|
||
| class ChannelTransforms: |
There was a problem hiding this comment.
Now you can divide this class into small pieces with two motivations.
- Small transformation script is easy to write a unittest. If there is a huge script, we can induce many edge cases.
- If some people want to customize a part of this script, one needs to prepare almost the copy of
ChannelTransforms, and there will be a lot of boilerplate code to manage. If the transformation is a collection of small pieces, we can reuse rest of it.
To me it looks like this class consists of 5 functionalities
- Convert
ParametricPulseintoWaveform(to_waveform) - Track phase and frequency of time t and add frame information to each play instruction (
track_frame) - Apply phase shift based on attached phase (
apply_phase_modulation) - Apply sideband modulation based on attached frequency (
apply_frequency_modulation) - Merge waveforms on the huge ndarray (
merge_channel_waveforms)
I think these transforms can be written as a separate functions rather than a huge script, i.e. passes. At this level of segmentation, we can flexibly build arbitrary transform chain.
For example, in the V2 pulse drawer, pass 3 and 4 are only called based on user preference (because some users don't want to apply these modulation) and 5 will be never called. I would write a visualizer_transform function by combining previous passes with passes 1-4 above (that is different version of target_qobj_transform).
Note that you may need validation for the program duration in the pass 5. Since channel waveform is usually complex128 datatype, the long array (e.g, if you schedule QV circuit, you'll get very long schedule) will consume the computer memory and it may hang up.
|
Hi @nkanazawa1989, thanks for your comments. I added
I was thinking about this and am not sure if this approach would be better than what
I wonder what you and @taalexander think about these. With regard to splitting functionalities,
I think 1 corresponds to Since a lot of scripts are already modified and moved around in this PR, I made minimal changes to the functions from the original |
Yes, agreed. The
Back to your original motivation, probably you just need to add Creation of these passes will be done in follow-up PRs if you will :) |
|
Now I see that this class should not be in
Should I add the |
|
Of course you can add the method to the |
Summary
I have written a function
get_channel_waveforminpulse/transforms/channel_transforms.pyto obtain the waveform on a PulseChannel.Details and comments
I have tested the function on predefined pulse schedules, specifically the
cx01andmeas0in the following code.It feels to me that the function should be a method of
Schedulerather than a standalone function in a standalone script. I would like to know what the maintainers think.