Remove platform start and stop

[stavros11]

Fixes #673. In the end I kept platform.connect() and platform.disconnect() and the same for instruments.
I also kept instrument.setup because it is used for loading the settings from the runcard (in serialize.py), however I removed platform.setup() as the loading of settings happens in the platform creation.

Since we are breaking the interface anyway, I also removed some set_*/get_* methods from the platform as they are not used much in qibocal anymore (with a very small exception) and the SPI driver as we have not used it for very long time and the driver may be outdated anyway (if we need it we can go back to the latest tag).

Some small updates in qibocal and qibolab_platforms_qrc are needed after merging this PR!

Checklist:

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

[alecandido]

Actually, are you sure do we need to retain setup()? To the best of my knowledge:

• only the LO are using it
• most of the others are calling it empty in the docstring (Qblox, Zurich & RFSoC)
• some are even calling it deprecated (QM & RFSoC)

[alecandido]

Can't you just set it in any case, even when self.sweetspot == 0?

Would it be wrong for some reason?

[alecandido]

My main doubt on the PR is just leaving the setup().

The rest is pretty much good to go, but here there is a further doubt that is related to a change quite unrelated to start(), stop(), and setup() in the first place.

[alecandido]

Do you believe this is actually required?

Here we are duplicating the information, in channel and qubit, and then we have to increase the code complexity to keep them in sync.

In general, it is good to have a single source of truth, and always read from there (currently, if the .sweetspot is updated afterward, ._flux get out of sync, unless properly corrected).

The problem is that the qubit depends on the channel, and not the opposite.
However, the qubit is the parameters' container, and the channel (and especially the port) is the one acting to send pulses.

So, eventually you need to play a pulse on a qubit, and this information goes down to the instruments (the .play() or .sweep() methods of the controllers, that we may eventually unify

qibolab/src/qibolab/instruments/qm/driver.py

Lines 111 to 112 in d881d1f

	def play(self, qubits, couplers, sequence, options):
	return self.sweep(qubits, couplers, sequence, options)

).
However, the instrument has no access to the channels, that's why is provided through Qubit. But in principle, we could even invert, and play the pulse on a Channel and a QubitId (even a Port is not unique, in case of multiplexing).

[alecandido]

I'm not sure that inverting the dependency is simple and the most effective strategy (esp. in the short term).
But I just see the duplication, and that you're trying to cope for it with syncing. And I know this is hard to maintain, in general.

[stavros11]

I also don't fully like this. In terms of this PR I did it to keep the behavior the same after the removal of platform.setup().

In general channel.offset can be different than qubit.sweetspot, for example if channel is not a flux channel or if we don't want to operate a qubit at its sweetspot. So it is not double source of truth, we are just assuming that we are operating all qubits at their sweetspots and we are setting the flux offset accordingly.

Other potential solutions I can think of are:

1. Drop offset from channels and modify drivers to use qubit.sweetspot as the offset of flux. This has many downsides as offset may be useful for non-flux channels and it will not be possible to operate qubits not at their sweetspot.
2. Drop qubit.sweetspot and just put the value on the offset directly as an instrument setting. Main downside of this is "political", people doing calibration may find it weird and the format of the runcards will change a bit so it should be communicated. Other than that that's not necessarily bad solution.
3. Keep both but the user has to set both manually. More flexible but will lead to repetition in the runcards, as the same value will be set twice in most cases. This is similar to the qubit frequency which is set as qubit.drive_frequency and also as the frequency of the native RX pulse.

[alecandido]

In general channel.offset can be different than qubit.sweetspot, for example if channel is not a flux channel or if we don't want to operate a qubit at its sweetspot. So it is not double source of truth, we are just assuming that we are operating all qubits at their sweetspots and we are setting the flux offset accordingly.

Even @hay-k mentioned this to me, and he mentioned that also @aorgazf asked for this feature (that in practice consists in doing something less).
But I agree that for this PR might be better to keep the same behavior (just because the PR is about something else).

Solution 1. I don't like, because as you pointed out they could be two different objects, and because we don't want to repeat over and over some decision in all the drivers.
Every entity should be represented, and we should make proper use of them. If we want to change the offset, let's do it at higher level.

As you said yourself, solution 2. is better software-wise. But if we calibrate the sweetspot, it has to be recorded somewhere.

Solution 3. is close to what we have, and it's the one I'd keep. I'd simply drop the automated sync at this point.
We can have a mechanism (a method? a parameter in other method?) to do that (set the channel.offset out of the qubit.sweetspot) if it's a convenient shortcut for reading from the one side and setting to the other. Or otherwise do it manually. But the decision should always be explicit (unless we have an offset setting stage, and we just keep sweetspot as the default - but this stage should be easily accessible by the user).

[stavros11]

But I agree that for this PR might be better to keep the same behavior (just because the PR is about something else).

Indeed, I opened an issue (#741) from this discussion. Let's come back to it in the next (or next next) milestone.

[PiergiorgioButtarini]

Actually, are you sure do we need to retain setup()? To the best of my knowledge:

• only the LO are using it
• most of the others are calling it empty in the docstring (Qblox, Zurich & RFSoC)
• some are even calling it deprecated (QM & RFSoC)

Actually also the Qblox modules are using the setup() in order to cache the values loaded from the runcard.

[stavros11]

• some are even calling it deprecated (QM & RFSoC)

Actually also the Qblox modules are using the setup() in order to cache the values loaded from the runcard.

And also in QM it is used as of #733, not in the Controller but in the individual devices (OPX/Octave), which is essentially similar structure with the Qblox modules.

The thing is that we need a way to deserialize the instruments section of the runcard YAML and because the options there are instrument dependent most likely this needs to be an instrument method. Actually, we will also probably need the inverse of this (instrument.dump()) which will serialize the settings, but I left its implementation for later.

It does not appear in Zurich because it is still following the old approach of hard-coding instrument settings in the create method:

qibolab/tests/dummy_qrc/zurich.py

Line 131 in cc8a1b4

channels["L3-31"].power_range = -15 # -15

and maybe RFSoC does not need any instrument settings (that could be calibrated).

[codecov]

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (9745314) 62.00% compared to head (fc55f79) 62.68%.

Files	Patch %	Lines
src/qibolab/instruments/qblox/controller.py	12.50%	7 Missing ⚠️
src/qibolab/couplers.py	83.33%	2 Missing ⚠️
src/qibolab/instruments/oscillator.py	77.77%	2 Missing ⚠️
src/qibolab/instruments/abstract.py	83.33%	1 Missing ⚠️
src/qibolab/instruments/dummy.py	50.00%	1 Missing ⚠️
src/qibolab/instruments/qblox/cluster_qcm_bb.py	0.00%	1 Missing ⚠️
src/qibolab/instruments/qm/driver.py	0.00%	1 Missing ⚠️
src/qibolab/qubits.py	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #739      +/-   ##
==========================================
+ Coverage   62.00%   62.68%   +0.67%     
==========================================
  Files          48       47       -1     
  Lines        6095     5930     -165     
==========================================
- Hits         3779     3717      -62     
+ Misses       2316     2213     -103     

Flag	Coverage Δ
unittests	62.68% <69.81%> (+0.67%)	⬆️

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.

[alecandido]

Actually, we will also probably need the inverse of this (instrument.dump()) which will serialize the settings, but I left its implementation for later.

These could be .load() and .dump() (like in json stdlib module, and also PyYAML and many others), but I thought that most of it was handled by the ...Settings classes, and thus being done during instantiation of the instruments (i.e. during __init__, and uplaoded during connect, rather than in a separate stage).

[stavros11]

These could be .load() and .dump() (like in json stdlib module, and also PyYAML and many others), but I thought that most of it was handled by the ...Settings classes, and thus being done during instantiation of the instruments (i.e. during __init__, and uplaoded during connect, rather than in a separate stage).

Then I will rename setup() to load() to avoid confusion with the old setup().

Settings are indeed uploaded during connect but loaded (from the runcard dict to the instrument object) using serialize.load_instrument_settings which uses instrument.setup. Handling in the Settings classes is not possible, because for example for Qblox, settings look like:

module_name:
    port_ name:
        setting_name: setting_value

so Settings are in the inner layer and do not have information about the ports. So it is probably more convenient to load at the module/instrument level (@PiergiorgioButtarini let us know if you have any input).

Loading in __init__ is also possible, but I am afraid that then initialization will become more complex (we are already passing addresses and names) so it may be easier to have a separate load/dump mechanism.

[alecandido]

Ok, I only had concerns related to .setup() and .flux property, and they have been discussed.

We could keep discussing .setup(), and decide whether or not we want to get rid of it, but I feel like it does not belong any longer to this PR (it became different in kind from .start() and .stop()).

It's good to go.

[alecandido]

Settings are indeed uploaded during connect but loaded (from the runcard dict to the instrument object) using serialize.load_instrument_settings which uses instrument.setup. Handling in the Settings classes is not possible, because for example for Qblox, settings look like:

I was imagining (and them proposing) __init__, not anything else (so ...Settings should have been used in there).

Loading in __init__ is also possible, but I am afraid that then initialization will become more complex (we are already passing addresses and names) so it may be easier to have a separate load/dump mechanism.

The idea was just to pass the corresponding runcard section, and deserializing the Settings inside, rather than outside. But I'm open for discussion about this, I'm not so strongly opinionated.

[stavros11]

Qblox and Zurich qpu tests are now passing on this branch. As soon as the CI passes, I will merge this.