Clear some clutter in the ZI driver

[hay-k]

The ZI driver code is unnecessarily complicated and entangled, which makes it very fragile, hard to read and change, easy to introduce bugs. This PR is an attempt to untangle and simplify the code.

What is changed:

• Most of the logic related to analyzing sweeps and deciding how to handle them is extracted into the ProcessedSweeps class, and the classify_sweepers function. This removes a lot of repetitive code scattered throughout the driver code, plus makes code more linear and easy to follow.
• The functionality of the classes ZhPulse, ZhSweeper, and ZhSweeperLine are combined into one. That lucky one is the ZhPulse. The other two are removed.
• Splitting a sequence into subsequences is unified in one place. Previously, for each experiment signal the splitting would happen separately, which gave rise to a lot of code duplication, and need to have functions like find_subsequence_finish, and weird phenomena like self.sequence[f"readout{q}"] returning non empty list of measurements when q is a coupler. Now all signal lines are split together and they are represented together with the measurements responsible for split via the newly introduced SubSequence class.
• For laboneq logical signal names the names of channels are used (with some caveats related to measurement and acquire channels) directly, instead of inventing yet another set of names.
• The need to access Qubit and Coupler object throughout the driver code is greatly reduced. This moves the driver towards a direction where it is easier to implement upcoming plans for logical channels.
• Methods play_sweep_select_single and play_sweep_select_dual are unified into the method play_sweep and generalized - in principle the driver should be able to handle any number of sweeps, not only 1 and 2.
• Removed the fast_reset functionality. Such functionality does not belong to the driver, and should be handled either in higher levels of qibolab, or even outside of it. I believe there is no known user of that functionality for now, hence I thought I could afford removing it for now and then later reintroducing it properly.
• A lot of redundant instance variables are removed.
• Some outdated code is removed.
• Minor esthetic changes here and there

Bugfixes:

• Fixed sweeper rearrangement not working for experiment with more than 1 qubits.
• Fixed readout pulse amplitude not being scaled up after the sweeper values are normalized.

There are still some things that could be polished, however I tried avoiding to touch everything as much as possible. The structure of the code is largely still the same. The goal was to rewrite the minimum possible as an intermediate step towards the upcoming introduction of logical channels in qibolab. Now it should be easier to adapt to those new concepts.

TODO: still need to fix the tests

Checklist:

• [ ] Reviewers confirm new code works as expected.
• [ ] Tests are passing.
• [ ] Coverage does not decrease.
• [ ] Documentation is updated.

[alecandido]

Thanks @hay-k for start polishing it, it was due since long.

I assigned @Jacfomg as a reviewer, because he's the original author of these drivers, so it's interesting to have his opinion already at an early stage (though I'm pretty sure you already discussed with him about the topic).

[alecandido]

Btw, most likely the module could also be radically rearranged, so feel free to start simplifying things adiabatically, but also start thinking about how things could be better organized in case you started from scratch.

A further point, that I might have already remarked, is that laboneq is doing many things that are also done by Qibolab. So, for as long as its abstractions match, and it's convenient, it is fine to keep using them. But as soon as the conversion from one to the other start being verbose and not that intuitive, we may move to the lower level Zurich drivers.

[alecandido]

(Trivial comment: a file of >1k lines has to be splitted asap)

[alecandido]

@hay-k what about this? https://github.com/qiboteam/qibolab/blob/0d27c8b1e47f22f7417c606070dcf3f308d4fec4/src/qibolab/instruments/zhinst.py#L78 (and the related lines below)

I'm not sure how it works: to my naive understanding, it should be a unique identifier of the pulse within the experiment, such that you construct your library of pulses to be played in a single batch, and then you download all the results in a single transaction.

I expected them to be overly specific, and that they should have been deduplicated. But this ID seems to me too generic, just specifying the pulse qubit and type (i.e. pulse.type.name.lower(), corresponding to the channel type, in the 0.1 pulse style).

How is it possible that there is no clash?

[alecandido]

@hay-k a few simple requests, before an actual review:

• first congrats: I'm not sure (yet) everything is compatible with the previous version, but if it is you achieved a 30% reduction of the amount of code for free! 🎉
• this file is still 1k lines, spanning everything that is required for ZI - would you try to split it in smaller ones, on the model of QM?
  • I'm asking now because this would facilitate the review (the changes are anyhow big, so moving the code to other files should not be terrible at this point)
• tests are failing, could you fix them?

[hay-k]

this file is still 1k lines, spanning everything that is required for ZI - would you try to split it in smaller ones

I was thinking to do it in a separate PR, so that the diff for this one looks a bit nicer. But if you think splitting already now helps for the review, I will do now.

tests are failing, could you fix them?

Sure, I am on it.

[alecandido]

I was thinking to do it in a separate PR, so that the diff for this one looks a bit nicer. But if you think splitting already now helps for the review, I will do now.

I had the same thought, but after opening the Files changed tab I realized that I'm getting very little from this diff anyhow, since there are more changed lines than unchanged ones...

[codecov[bot]]

Codecov Report

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

Project coverage is 66.22%. Comparing base (c19ca70) to head (92f9640). Report is 3 commits behind head on main.

Files	Patch %	Lines
src/qibolab/instruments/zhinst/executor.py	82.07%	57 Missing :warning:
src/qibolab/instruments/zhinst/sweep.py	88.40%	8 Missing :warning:
src/qibolab/instruments/zhinst/pulse.py	84.09%	7 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #797 +/- ## ========================================== - Coverage 66.51% 66.22% -0.29% ========================================== Files 50 54 +4 Lines 6063 5875 -188 ========================================== - Hits 4033 3891 -142 + Misses 2030 1984 -46 ``` | [Flag](https://app.codecov.io/gh/qiboteam/qibolab/pull/797/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/797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `66.22% <83.71%> (-0.29%)` | :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.

[hay-k]

@alecandido Most of the comments you have make sense, and I agree that a lot of things could still be improved. As I mentioned in the description of the PR, I did not attempt to perfect everything. I had to stop somewhere. The main purpose was to extract usage of Qubits and Couplers away from the driver code, and centralize the way sweeps are handled. This is pretty much preparation for the other upcoming refactor related to the introduction of logical channels. I would avoid refactoring more than this now, because most of the code will be refactored again soon. I will address selected comments that are more relevant to the main goal of this PR, but will leave the rest for future.

[alecandido]

I would avoid refactoring more than this now, because most of the code will be refactored again soon. I will address selected comments that are more relevant to the main goal of this PR, but will leave the rest for future.

I agree, especially concerning aggressive refactoring, like the denesting.

I'm having my first dive into this code as a whole, so I'm annotating down ideas for improvements. It is perfectly fine to leave the comments that are sensible but not urgent where they are. Just apply solutions for the low-hanging fruits, and close those that you believe to be not suitable. Those left open could be used as a reference (i.e. a starting point) for further discussions, after this PR.

[Jacfomg]

@hay-k should we try to calibrate a qubit with this driver as well ?

[hay-k]

@hay-k should we try to calibrate a qubit with this driver as well ?

@Jacfomg We can try. So far I have tested the following experiments (i.e. checked that they produce the same results as main), but without a strict intention to calibrate the qubit:

1. resonator spectroscopy
2. resonator punchout
3. qubit flux dependence
4. rabi amplitude
5. rabi length
6. ramsey (both with and without unrolling)
7. single shot readout
8. allxy
9. T1

[Jacfomg]

@hay-k just to check, I found while closing https://github.com/qiboteam/qibolab_platforms_qrc/pull/121 some improvement that may be relevant here or on the following. Could we move power_range into the parameters.json like QM ?