Replace `pulse.start` with delays and refactor native.py

[stavros11]

@alecandido @hay-k following what we discussed last week, the goal of this PR is to replace pulse.start with a new Delay object. In the next days I will try to propagate the changes to native, platform and eventually the QM driver as a first driver test (although for this I'd prefer to have #733 and #738 available). In the meantime, feel free to have a look and let me know if you have any suggestions so that this moves towards a direction that all of us like (as much as possible).

Here is a summary of the affected objects so far:

Delay

New class added to represent delays. Decided to go with a new object, completely seperate from Pulse because:

• it is more straightforward in terms of usability (delay pulse shape does not sound very nice),
• in most (if not all) instruments delays correspond to a different instruction than the one used to play pulses, unless they are implemented with zero waveforms, which is the wrong way. There may be some exceptions to this rule, for example QM does not support delays < 16ns and in such cases other workarounds need to be found, but these can be handled internally in the driver.

Pulse

Dropped the following methods/properties:

• [X] finish: not applicable if Pulse does not have start (dropped).
• [x] global_phase: requires start so it should be handled by the sequence but we can probably drop altogether because it is only used for software modulation.
• [ ] phase: should we rename relative_phase to phase?
• [x] is_equal_ignoring_start: it can probably be replaced by dataclass autogenerated __eq__.

PulseSequence

Some implementations will depend on the choice between (internal vs external) pulse sequence (internal=channel attribute, sequence is just list / external=sequence has a list for each channel)

Require modification:

• [x] duration: implementation depends on (internal vs external). I did a temporary implementation based on the current internal representation of channels, essentially by constructing the external.
• [X] start: probably not useful (dropped)
• [X] finish: equivalent to duration if we drop start (dropped)
• [X] get_pulse_overlaps: implementation depends on (internal vs external) (dropped)
• [x] seperate_overlapping_pulses: same as above (used by qblox)

Align (not implemented)

A suggestion of an additional non-pulse instruction which can be used to synchronize different channels, without explicit use of delays. For example, the README example would look like:

sequence.append(Pulse(duration=4000, ... channel=1))
sequence.append(Align(1, 2))
sequence.append(ReadoutPulse(... channel=2))

I did not implement this because it may be biased towards QM and complicate other drivers, so I would prefer to wait for feedback first.

---

Sweeper

• [X] Removed Parameter.start

Native gates

• [X] Removed NativePulse, NativeSequence, etc. as they are duplicates of Pulse and PulseSequence.
• [X] Moved serialization related methods (from_dict and raw) to serialize.py since they are more associated with the YAML/JSON runcard rather than the qibolab API.
• [ ] How to handle pulse virtual-Z phases?

Platform

• [X] Removed start from platform.create_* methods.
• [x] How to handle pulse phases? I am temporarily modifying the phase of pulses after creation but this is not ideal particularly if we want Pulse to be frozen.
• [x] Update unroll_sequences.

[alecandido]

New class added to represent delays. Decided to go with a new object, completely seperate from Pulse because:

Fine, but then create a union (at least typewise, like ... = typing.Union[Pulse, Delay]) to represent the sequence element, such that a sequence will still be a "homogeneous" list of elements (of course it is not truly homogeneous, because there is a union involved, but we'll be able to reference the sequence element type with a given name).

• There may be some exceptions to this rule, for example QM does not support delays < 16ns and in such cases other workarounds need to be found, but these can be handled internally in the driver.

Fine, these are the things that should be handled internally anyhow.

[alecandido]

• [ ] phase: should we rename relative_phase to phase?

Fine for me, there is no another meaningful phase for a single pulse

• [ ] is_equal_ignoring_start: it can probably be replaced by dataclass autogenerated __eq__.

Yes, that was the goal since #683

[alecandido]

• [ ] finish: implementation depends on (internal vs external). I did a temporary implementation based on the current internal representation of channels, essentially by constructing the external.
• [ ] start: probably not useful
• [ ] duration: equivalent to finish if we drop start

Personally, I would only keep duration.

• [ ] seperate_overlapping_pulses: same as above (we could drop these methods?)

I would, because with the logical channels there should be no overlap. In case this will become useful for validation, and when it will be needed, we may reintroduce it. But for as long as unused, let's drop it.

[alecandido]

Align (not implemented)

A suggestion of an additional non-pulse instruction which can be used to synchronize different channels, without explicit use of delays. For example, the README example would look like:

I did not implement this because it may be biased towards QM and complicate other drivers, so I would prefer to wait for feedback first.

This could be part of what I called the "high-level Pulse API". Let's just stick to stabilizing the low-level one first, and rediscuss the first right after.

[hay-k]

I agree with all things said above. Just wanted to mention, that an alternative to accepting union Pulse | Delay (and in the future maybe Align as well) could be having specialized methods for sequences. I.e. sequence.append(channel, pulse) can append only a Pulse, a delay can be added by sequence.delay(channel, duration), etc. The Delay class would still be needed to represent the delay internally. The benefit is less confusing user API, the drawback is that it is not clear how this will work with sweeps (i.e. what if you want to sweep a delay), at least with their current implementation.

[alecandido]

The benefit is less confusing user API, the drawback is that it is not clear how this will work with sweeps (i.e. what if you want to sweep a delay), at least with their current implementation.

I would expose the internal API as it is. If we want to add convenience methods in the "high-level" API that's fine, but I would not overcomplicate the basic one.

The Delay class would still be needed to represent the delay internally.

Exactly. And the main deal is not the .append(Pulse | Delay), but rather the dict[Channel, list[Pulse | Delay]]. Once we agree about the second, the first is there for free (part of the list API anyhow).

[alecandido]

(and in the future maybe Align as well)

I would compile the Align in Delays.

However, we could decide if this should happen in two steps (first define the whole sequence, then compile to the machine one) or directly at insertion level. In case we'll go for the two steps, I'd also have two classes:

class Sequence(dict[Channel, list[Pulse | Delay]):
    ...

class SequenceBuilder:
    ...

    def compile(self) -> Sequence:
        ....

[stavros11]

Fine, but then create a union (at least typewise, like ... = typing.Union[Pulse, Delay]) to represent the sequence element, such that a sequence will still be a "homogeneous" list of elements (of course it is not truly homogeneous, because there is a union involved, but we'll be able to reference the sequence element type with a given name).

For sure we can do that, I just did not do it yet because I did not find any place where this type is required (at least yet). For example PulseSequence.append is not explicitly defined here because it is inherited from list, but I guess this will change in #792 and at that point we can define the type.

Fine for me, there is no another meaningful phase for a single pulse

Will do the rename (relative_phase -> phase) but at a later change and maybe in a different PR because a different phase already existed in the pulse and we should be careful when we update it (particularly in the drivers). That is not very critical and easy to update anyway.

On this point, it is worth noting that yesterday we had a discussion with @hay-k about whether phase (currently relative_phase) belongs to the Pulse or no. In the end we decided the keep in Pulse for now but we (especially me) need to understand in more detail how it is used in the code.

A potential argument for not keeping in the pulse is that in QM (and I think also Qblox), phases are controlled via separate sequencer instructions instead of encoding them to waveforms. Therefore, a X-pulse and a Y-pulse (=X + pi/2 relative phase) is essentially the same pulse used in a different way in the program:

play("x", "channel_id")

for X and

frame_rotation_2pi(0.25, "channel_id")
play("x", "channel_id")
reset_frame("channel_id")

for Y. In both cases we only need to upload the X-pulse with zero phase.

I would compile the Align in Delays.

I am not sure about this, because if all instruments provide such instruction, then it is probably simpler from our side to expose it to the pulse API and just pass it directly to the instrument, instead of compiling it ourselves. Then the question is if all instruments support it. I only know that QM does and I believe ZI provides something similar (I have heard of some play_after) but I am not sure if the correspondence between the two is simple.

Another issue that I encountered with QM in particular, is that depending on the rest of the program, Align may not be compiled (by their compiler) to the delays you expect and in some cases there may be weird overheads. We certainly cannot control that but should keep in mind if we decide to expose Align to our interface. Or just stick to delays and never use Align at the low level (requires a bit more work).

Regardless of compiling or no, this instruction would be useful to have in some cases such as the unrolling function https://github.com/qiboteam/qibolab/blob/13bdcb4644c26dd48097de2a4dc0211349fce9fa/src/qibolab/platform.py#L25 but we could consider this at a later stage.

[alecandido]

I am not sure about this, because if all instruments provide such instruction, then it is probably simpler from our side to expose it to the pulse API and just pass it directly to the instrument, instead of compiling it ourselves. Then the question is if all instruments support it. I only know that QM does and I believe ZI provides something similar (I have heard of some play_after) but I am not sure if the correspondence between the two is simple.

Another issue that I encountered with QM in particular, is that depending on the rest of the program, Align may not be compiled (by their compiler) to the delays you expect and in some cases there may be weird overheads. We certainly cannot control that but should keep in mind if we decide to expose Align to our interface. Or just stick to delays and never use Align at the low level (requires a bit more work).

Conceptually, Align has to be realized as a bunch of Delays, because it does not make sense at low level to have some kind of synchronization mechanism, when you can establish the synchronization ahead of time.

Even though all APIs were exposing an interface for that, it could be still simpler to use Delays. In the case of low-level Aligns, if you have N drivers you should implement the 2N translations (N for Delay and N for Align). But compiling the Aligns to Delays is just N + 1, since the conversion would be common to all of them.

[alecandido]

On this point, it is worth noting that yesterday we had a discussion with @hay-k about whether phase (currently relative_phase) belongs to the Pulse or no. In the end we decided the keep in Pulse for now but we (especially me) need to understand in more detail how it is used in the code.

A potential argument for not keeping in the pulse is that in QM (and I think also Qblox), phases are controlled via separate sequencer instructions instead of encoding them to waveforms. Therefore, a X-pulse and a Y-pulse (=X + pi/2 relative phase) is essentially the same pulse used in a different way in the program:

About this I'm not sure either. At the moment, it could be fine both ways.

However, consider that this would be in a completely opposite direction to Align: while Align is a possible useful but not-strictly-necessary feature, what you're saying is that the same is true for Pulse.phase.

To avoid this kind of confusion, maybe we could split user's pulses from executed pulses (or something similar). Keep the first just as an interface, which could be featureful, and the second as minimal as possible. But this depends on what we want Qibolab to be: if it's only the standardization layer for Qibocal, maybe it should just be minimal. If there will be other users around, then it would be useful for them to avoid repeating layers that would be implemented in Qibocal otherwise.

[stavros11]

However, consider that this would be in a completely opposite direction to Align: while Align is a possible useful but not-strictly-necessary feature, what you're saying is that the same is true for Pulse.phase.

This point is not correct (or I don't understand it properly). Align is indeed a not-strictly-necessary (aka optional) feature since any Align can be compiled to Delay. This is why I will probably not implement it at this stage. Phase on the other hand is a strictly-necessary (aka required) feature, otherwise I think we cannot do Y rotations. We have the option to keep it in Pulse or somewhere else, but we strictly need it somewhere.

[alecandido]

I believe it's just the perspective that is shifted: I'm thinking about a minimal hardware-close representation, and in this sense we both don't need optional features for that, but also the required ones could be presented in a more abstract or explicit way.

The rationale to move the phase out of Pulse is being closer to hardware. Instead, introducing Align would make it closer to the user. These two directions are not exactly opposite to each other, but almost (there are cases in which you could be closer to both, if you start from very far...).

That's why I was pushing for "layers": to keep one as close as possible to one end, and the other one to the other. Since the two of them will be both in Qibolab and both standard, it will be easier to go from one to the other (avoiding repetitions).

flowchart LR
  u1[user 1] --> up[user-pulse API]
  u2[user 2] --> up
  u3[...] --> up
  qibocal["(qibocal)"] --> up
  up --> llp[low-level pulses]
  llp --> Zurich
  llp --> qm[Quantum Machine]
  llp --> Qblox
  llp --> driver[...]

My other point was to start from the low-level, and postpone UI to a later effort (as soon as we complete the other one), since we will benefit of the low-level for all the other internal operations.

[alecandido]

@stavros11 since it was getting complex (after the recent 0.2 updates) I rebased on top of 0.2, but I had to make some manual choices, so please check that the diff is still meaningful.

I know it could be annoying, but since we have to deal with the parallel development in main(=0.1) and 0.2 lanes, please try to stick to rebase (instead of merging) within the 0.2 domain, at least it will remain a clean set of diffs on top of each other.

(@hay-k, I tag also you to be aware, no one else is working on 0.2)

[stavros11]

I have skipped temporarily the instrument tests and added very few pylint exceptions in instruments (in d0a1567453703fd363410abb2756d432aa685894) since I am not going to touch drivers in this PR but I would still like to make the rest (and the CI) pass.

Still TODO before this is ready for review:

• [ ] Compiler has some bug because related tests (and doctests) are not passing.
• [ ] Find a way to handle virtual-Z phases in two-qubit native gates while preserving their order (and unskip runcard dump test).

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 94.46494% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 49.63%. Comparing base (7993aec) to head (3921bef).

Files	Patch %	Lines
src/qibolab/platform.py	90.66%	7 Missing :warning:
src/qibolab/instruments/rfsoc/convert.py	33.33%	2 Missing :warning:
src/qibolab/pulses/plot.py	88.88%	2 Missing :warning:
src/qibolab/serialize.py	96.96%	2 Missing :warning:
src/qibolab/compilers/compiler.py	96.15%	1 Missing :warning:
src/qibolab/instruments/zhinst/sweep.py	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## 0.2 #789 +/- ## =========================================== - Coverage 65.58% 49.63% -15.95% =========================================== Files 58 58 Lines 5654 5573 -81 =========================================== - Hits 3708 2766 -942 - Misses 1946 2807 +861 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/789/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibolab/pull/789/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `49.63% <94.46%> (-15.95%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[stavros11]

@alecandido @hay-k tests are now passing, so I will mark this as ready for review. Because the diff is quite huge (sorry for this - may be because I ended up doing two things: removing start and refactoring native.py), I will summarize here the main changes and decisions I took that are up for discussion:

1. pulses/pulses.py (*): A new Delay dataclass was added and start, finish, global_phase, phase was dropped from the Pulse
2. pulses/sequence.py: start, finish and some other methods that were using pulse.start but were not used anywhere else in the code are dropped from PulseSequence. PulseSequence.seperate_overlapping_pulses remains because it is used somewhere in qblox, but I did not update it (it is still using pulse.start so it will fail). The idea is to postpone this until we refactor qblox and hopefully this won't be needed anymore so we can drop (maybe I should open issue).
3. native.py: Most was dropped as NativePulse/NativeSequence are now replaced by Pulse/PulseSequence. The serialization related methods are moved to serialize.py.
4. serialize.py: A bunch of new methods that used to be in native.py are added. Currently this contains everything that is specific to the json format we are using for the platform runcards. The rest of qibolab is independent of this.
5. platform.py: unroll_sequences was updated to use delays instead of starts. Some temporary methods (_set_channels_to_*_gates) were added temporarily (explained in docstring) but should be removed after #792 (or whenever we drop pulse.qubit, which I understand will happen at some point).
6. compiler.py: Compiler was updated to use delays instead of starts and the new VirtualZ (*).

(*) In ed1a4ddfc876e0d76ee1c86ae8cd4f477e5b771d I decided to add a new PulseType, the VirtualZ. I also implemented it as a separate class (similar to Delay) but this is not strictly necessary, it can be easily replaced by an existing Pulse (with a different type), in that case some attributes (eg. amplitude, frequency) will be redundant. Of course this is open to discussion, but to me it seems to simplify the compiler and the deserialization of two-qubit native gates and generally the handling of virtual-Zs is now more transparent than before that was hidden in some dictionaries being passed around by various methods. If we don't wan't to implement VirtualZs at the driver level, we can have an additional sequence.drop_virtualz that gives the equivalent sequence without VirtualZs and the phases of other pulses appropriately adjusted.

The rest of changes are trivial adjustments to make the code work with the new Delay and native refactoring. Instruments are not updated and corresponding tests are skipped.

I will start refactoring the QM driver to see how it works with the Delay. In the meantime it would be nice if you can have a look at this.

[alecandido]

Great, thanks!

I'll try to review this soon.

I will start refactoring the QM driver to see how it works with the Delay. In the meantime it would be nice if you can have a look at this.

If you can, just do it in a further PR, since you don't need them for passing the CI.

[stavros11]

Thanks @alecandido for the extensive review. I responded to some of the comments above. These are the ones that I kind of disagree or needed to provide some explanation. For the rest, I tend to agree and I am planning to implement and let you know if some issue comes up.

Two other major things I didn't comment on above are the following:

1. Whether Delay and VirtualZ should be new objects or Pulses. Several review comments are related to that. I see the advantages and disadvantages of either approach but let me propose to try to discuss this tomorrow (or in the next days) in person and try to reach a final decision.
2. Some contents of serialize.py can indeed be moved to constructors and dump methods of other objects. Again several review comments are on that. Here, my dream would be to simplify all qibolab objects to the extent that their serialization/deserialization is trivial (via dataclasses or some other functionality) so that we don't need the serialize.py. Until then, I decided to move the serialization to serialize.py so that all the "mess" that is associated with the qibolab_platforms_qrc runcard (json) format is in a single file and not spread around the code. Of course this is not necessary (and again up to discussion) but may simplify our life while refactoring.

[stavros11]

@alecandido this branch has been rebased on top of 0.2 (which is now on top of current main) and tests are passing locally. I will soon start implementing some of your comments above, but feel free to put your branch on top of it.

[stavros11]

I fixed some of the review comments. As discussed with @alecandido I will skip the following as they are going to be addressed in #818 (or when the pydantic serialization is implemented):

• Dropping type from Delay and VirtualZ objects. This is related to how we are going to json serialize these objects. It may also affect some of the PulseSequence code, because it is calling pulse.type and now pulse: Pulse | Delay | VirtualZ
• Changes related to serialize.py. These should also be taken care of the serialization.

Other than these, this should now be ready. Tests are passing but instrument tests are skipped until we update the drivers (I would wait for #792) for that.