Lift sequence

[alecandido]

In #970 I'd like to move towards more structured channel IDs.
Thinking about that, I considered that the main customer of the channels, other than the platforms (and of course the drivers), is the PulseSequence itself, that is now a combination of channels and pulses (as it is clear from _Element).

Since channels are an important constituent of the sequence, and they are not constituent of pulses, I decided it was more fair to lift the sequence out of the pulses submodule. Pulses are a thing on its own, the variables we use for our "instructions". A sequence is a program, i.e. a list of instructions.

(for the time being, an "instruction" is just the pulse + channel association, but we're about to insert Align and Acquire, that are not really pulses, as Delay and VirtualZ are already somewhat different, and then go for further instructions in #917 - they could also go fully outside the .pulses subpackage)

This PR is standalone because, despite there is almost no substantial change, it is generating a huge diff, and I didn't want to mix in anything else.

[codecov]

Codecov Report

Attention: Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.

Project coverage is 41.91%. Comparing base (b0ba6e4) to head (0ac2260).
Report is 8 commits behind head on 0.2.

Files	Patch %	Lines
src/qibolab/instruments/icarusqfpga.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              0.2     #986      +/-   ##
==========================================
+ Coverage   41.79%   41.91%   +0.12%     
==========================================
  Files          79       79              
  Lines        5451     5464      +13     
==========================================
+ Hits         2278     2290      +12     
- Misses       3173     3174       +1     

Flag	Coverage Δ
unittests	41.91% <94.73%> (+0.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stavros11]

Since the serialization has been finalized, we can probably also merge this. I have not checked every single file in detail, but it seems that it is only the lift and the rest of diff is just a consequence of that without anything new.

By the way, I believe sometime ago you proposed to rename PulseSequence to just Sequence. This could be a good place to do it, since it would produce a similar diff. Personally, I don't have strong opinion on the matter, of course Sequence would be simpler but I leave it up to you.

Since channels are an important constituent of the sequence, and they are not constituent of pulses, I decided it was more fair to lift the sequence out of the pulses submodule. Pulses are a thing on its own, the variables we use for our "instructions". A sequence is a program, i.e. a list of instructions.

That's a fair argument, so I would agree with the lift. However, there is a small asymmetry in how the PulseSequence is currently treating channels and pulses, since for channels it only stores the id (name), while for pulses the whole object. In that sense one could argue that is more related to pulses, than channels. Of course, this is because for channels we have an external database (Platform, Qubit and Instrument) that contain the channels and allows us to reference them by name elsewhere. For pulses there is no such external storage, so the PulseSequence takes care of it. We could design things in a way so that pulses are stored in a different place and PulseSequence only contains their ids. In that case PulseSequence would really only contain information about the scheduling of instructions. In fact, this is what happens in QUA.
(this is just a note, not arguing that we should do it)

[alecandido]

By the way, I believe sometime ago you proposed to rename PulseSequence to just Sequence. This could be a good place to do it, since it would produce a similar diff. Personally, I don't have strong opinion on the matter, of course Sequence would be simpler but I leave it up to you.

I actually gave up, because of the standard library Sequence. It could be confusing to have the exact same name.

[alecandido]

That's a fair argument, so I would agree with the lift. However, there is a small asymmetry in how the PulseSequence is currently treating channels and pulses, since for channels it only stores the id (name), while for pulses the whole object. In that sense one could argue that is more related to pulses, than channels. Of course, this is because for channels we have an external database (Platform, Qubit and Instrument) that contain the channels and allows us to reference them by name elsewhere. For pulses there is no such external storage, so the PulseSequence takes care of it. We could design things in a way so that pulses are stored in a different place and PulseSequence only contains their ids. In that case PulseSequence would really only contain information about the scheduling of instructions. In fact, this is what happens in QUA.
(this is just a note, not arguing that we should do it)

This is something we could think about, especially for 0.3, since when we'll have a full-blown program, it might be handy to define the pulses separately (as in QUA, we're just going in the same direction).
However, I would not commit to it for 0.2, since we're not gaining much advantage.

In a sense, above, I spelled out the abstract motivation. Its concrete manifestation are the imports: it's impossible that everything is fully self-contained, we need references to each other. But I preferred pulses not to know about channels, and that's why I wanted to join them "one level above".
In any case, even this reasoning has an objective aim, and an arbitrary component. I don't have a fully automated criterion to decide what to do.

[stavros11]

Thanks for the rebase. I updated the imports in qiboteam/qibocal#809 to sync with this and confirmed that the single shot routine still works on qw11q, so it should be fine to merge.

However, I would not commit to it for 0.2, since we're not gaining much advantage.

Certainly not for 0.2, the comment was mostly to say what I thought while reading the PR, we don't need to implement anything related. I am not sure about 0.3, but it is also early to think about this, let's see how 0.2 will look in the end and go from there.