search

qiboteam / qibolab

Quantum hardware module and drivers for Qibo.
https://qibo.science
Apache License 2.0
42 stars 13 forks source link

Drop `SweeperType` #540

Closed andrea-pasquale closed 1 week ago

andrea-pasquale commented 1 year ago

I think that it could be useful to make use of SweeperType introduced in https://github.com/qiboteam/qibolab/pull/459 for all drivers. In this way we could avoid to define global convention for each parameter that we sweep. I know that the rfsoc driver already supports this while in qblox this is currently being implemented in #518.

@Jacfomg @stavros11 could you please port this feature also in QM and Zurich?

Jacfomg commented 1 year ago

It's on my todo list #535. But, I will prioritize doing things with the chip before it goes away.

stavros11 commented 1 year ago

@Jacfomg @stavros11 could you please port this feature also in QM and Zurich?

Yes, I am also planning to do it.

andrea-pasquale commented 1 year ago

Perfect, thank you!

hay-k commented 6 months ago

@stavros11 @Jacfomg @andrea-pasquale I think this issue should wait until qibolab 0.2 is ready. In 0.2 we are aiming for a structure of qibolab with clearly defined layers (e.g. see https://github.com/qiboteam/qibolab/pull/792). The layer that is instrument specific shall not know anything about how to resolve sweeps. This resolution shall be done in the higher level before requesting execution from instruments. This means that sweep resolution will happen in one centralized place and should not be implemented inside each device specific code.

alecandido commented 6 months ago

I fully agree with the first part.

However, how to implement the sweep (how to transfer the parameters, and possibly further details about the sweeper implementation) will of course be instrument specific. But the most we can implement in the common layer, the better it is.

If there is enough consensus, change the assigned milestone.

andrea-pasquale commented 6 months ago

Fine also from my side, the motivation to open the issue was to have a common interface in Qibocal, given that we already making use of SweeperType. If things change in qibolab 0.2 I am perfectly fine.

stavros11 commented 6 months ago

Since everyone agrees, I moved this to the 0.2.0. And I also agree with lifting as much code as possible to the common layer.

alecandido commented 4 months ago

Actually, is it convenient for the devices to use the SweeperType? I.e. can they make use of it to save device memory or message size? @stavros11 @hay-k @rodolfocarobene

The alternative would be to just use SweeperType.ABSOLUTE (and leave the user to do the sum/product of the array on their side).

rodolfocarobene commented 4 months ago

The main reason why sweepertype different from absolute exist is because of multiple qubits calibration. I can do a spectroscopy with the same sweeper on different qubits if it is a sweepertype.offset, not if it is absolute (not as easily at least).

This was already done before, but implicitly.

From the QICK point of view, there is no difference in memory usage or anything else. Actually what I implement is always an "absolute"-like sweeper

alecandido commented 4 months ago

The main reason why sweepertype different from absolute exist is because of multiple qubits calibration. I can do a spectroscopy with the same sweeper on different qubits if it is a sweepertype.offset, not if it is absolute (not as easily at least).

Ok, but then this doesn't have to be a driver-supported feature, but it could be even a Qibocal feature. I.e. Qibocal can translate a single offset-specified sweeper in different absolute sweepers for Qibolab. Isn't it?

stavros11 commented 2 weeks ago

Actually, is it convenient for the devices to use the SweeperType? I.e. can they make use of it to save device memory or message size?

To answer this, SweeperType may actually be convenient for QM and could be used to save both device memory and communication size.

Following @rodolfocarobene comment, the main use of SweeperType was making parallel sweeps on different qubits. For example, using SweeperType.OFFSET you can sweep the frequency on multiple qubits using the same range but centered around a different value for each qubit. Since 0.2 provides a proper interface for parallel sweeps (which was not available in 0.1), multiple qubit sweeps can now be done using that and the SweeperType is not strictly needed in terms of API.

However, regarding QM in particular, parallel sweeps are compiled using the for_each_, which according to the docs is less efficient than for_ (and from_array) used for non-parallel (single) sweeps. Furthermore, looking at the parallel sweep example in https://github.com/qiboteam/qibolab/pull/1008#issuecomment-2313715249, it is clear that for parallel sweeps we need to upload the whole array of values we are sweeping over to the instruments, possibly requiring more memory and more communication compared to normal sweeps where we need to upload only three values per sweeper (start, finish, step), regardless of the total number of values. I have not benchmarked what is the actual difference in execution time in practice yet, but I would expect it could be relevant for large multi-dimensional sweeps.

Potential solutions for 0.2:

  1. Drop SweeperType, convert all instrument sweepers to ABSOLUTE and ignore potential performance drop in QM.
  2. Follow 1, but implement improvements in the QM driver that convert for_each_ sweeps to for_ whenever this is possible. This may involve some "guessing" that won't be very elegant in terms of code. The improvements are internal so could even be postponed to 0.2.*, but would be interesting to have an idea how they'd look and if they are relevant, before we fix the interface.
  3. Keep SweeperType and implement them in QM and all other drivers.
alecandido commented 2 weeks ago

Following @rodolfocarobene comment, the main use of SweeperType was making parallel sweeps on different qubits. For example, using SweeperType.OFFSET you can sweep the frequency on multiple qubits using the same range but centered around a different value for each qubit. Since 0.2 provides a proper interface for parallel sweeps (which was not available in 0.1), multiple qubit sweeps can now be done using that and the SweeperType is not strictly needed in terms of API.

I believe this was the kind of applications for which SweeperTypes were needed for. And in practice, they were used only in Qblox, IcarusQ, and the emulator https://github.com/search?q=repo%3Aqiboteam%2Fqibolab%20SweeperType&type=code the first one with a complex implementation (which was coupled to sweeper parameters - and I'm not exactly sure why, though I suspect it - but I know that in the end is always converting to (start, stop, increment) instructions at software level) and the other two with a trivial software implementation https://github.com/qiboteam/qibolab/blob/bbac7cf23e0d879f792a16f64e2f0f5e7b147a01/src/qibolab/instruments/icarusqfpga.py#L419-L423 which makes it useless, since it could be done on the Qibocal side (if needed).

However, regarding QM in particular, parallel sweeps are compiled using the for_each_, which according to the docs is less efficient than for_ (and from_array) used for non-parallel (single) sweeps. Furthermore, looking at the parallel sweep example in #1008 (comment), it is clear that for parallel sweeps we need to upload the whole array of values we are sweeping over to the instruments, possibly requiring more memory and more communication compared to normal sweeps where we need to upload only three values per sweeper (start, finish, step), regardless of the total number of values. I have not benchmarked what is the actual difference in execution time in practice yet, but I would expect it could be relevant for large multi-dimensional sweeps.

QM 0.1 is not even supporting sweeper types, and deciding the "type" based on the parameter. E.g. https://github.com/qiboteam/qibolab/blob/9bbbbe8c30bad4e99ba2cbbcb32ecb63147a43d4/src/qibolab/instruments/qm/sweepers.py#L97-L100 (this is the frequency, and it's a SweeperType.OFFSET, always - irrespective of the actual SweeperType set - but there is a similar for_ loop in all the other parameters)

So, since we'd really like to release 0.2 at this point, I would keep the minimal feature right now, and keep performance optimizations for later.

Potential solutions for 0.2:

  1. Drop SweeperType, convert all instrument sweepers to ABSOLUTE and ignore potential performance drop in QM.
  2. Follow 1, but implement improvements in the QM driver that convert for_each_ sweeps to for_ whenever this is possible. This may involve some "guessing" that won't be very elegant in terms of code. The improvements are internal so could even be postponed to 0.2.*, but would be interesting to have an idea how they'd look and if they are relevant, before we fix the interface.
  3. Keep SweeperType and implement them in QM and all other drivers.

I would keep 2. and 3. for 0.2.2 or later. In the meanwhile, just drop it, and follow the 0.1 approach. The idea is that we can make the SweeperType available in a later release (if worth for performances), and add a Parameter-dependent default. I.e. we'll put None, but if None we resolve as we're doing in the previous releases, even driver-dependent, if really needed (hopefully not, and we'll standardize the meaning across drivers before), but just avoiding breaking changes. It will be possible.

We also need to support (start, stop, step), because some instruments can only do that (e.g. Qblox, but even QICK). In 0.1, they are inferring these values as (vals[0], vals[-1], vals[1] - vals[0]), but we should define an interface for doing directly that, and avoiding passing the entire array if it's not possible to use it (I fear that the problem was that Sweeper may have been designed with QM in mind, which was more capable than other instruments).

I'd keep the 0.1 behavior even in 0.2, but let's switch input:

class Sweeper(Model):
    ...
    linspace: Optional[tuple[float, float, float]] = None
    values: Optional[npt.NDArray] = None
    ...

    @model_validator(mode='after')
    def check_values(self) -> Self:
        if linspace is not None and values is not None:
            raise ValueError("'linspace' and 'values' are mutually exclusive)
        return self

    @property
    def vals(self) -> npt.NDArray:
        if self.values is not None:
            return self.values
        return np.linspace(*self.linspace)

(Pydantic syntax, but trivial to translate in dataclasses)

andrea-pasquale commented 2 weeks ago

Personally I would be in favor of this.

  1. Drop SweeperType, convert all instrument sweepers to ABSOLUTE and ignore potential performance drop in QM.

Since having OFFSET and FACTOR it is often confusing for qibocal users. Moreover, I believe that the original idea was o be able to perform single qubit calibration experiments on multiple qubits simultaneously... Although this is for sure interesting, it shouldn't be the default. A possible solution for qibocal could be to adjust the parameters to have ones for each qubit.


  - id: resonator spectroscopy low power
    operation: resonator_spectroscopy
    targets: [0, 1]
    parameters:
      freq_width: { 0 : 5_000_000, 1: 4_000_000}
      freq_step: { 0 : 100_000, 1: 200_000}
      power_level: low
      phase_sign: true
      nshots: 1024

This is just a first attempt at a new interface. But I think that it would look much clearer compare to what we are doing now. Of course in this particular case you will need to move from freq_width and freq_step to freq_min, freq_max and freq_step .

alecandido commented 2 weeks ago

Since having OFFSET and FACTOR it is often confusing for qibocal users.

It's not that simple, but we can keep the substance. E.g. while you're sweeping a pulse frequency, you almost never want to really specify the absolute value (~GHz) but always the offset (~MHz). And this is what is being done, simply, the drivers are deciding by themselves (each separately) which is the meaningful choice for each parameter.

We could really convert it to absolute, but then Qibolab would only deal with ~GHz frequencies, and the conversion should be done inside Qibocal, fishing the base value from wherever it is.

For the time being, there will only be QM around, so it's easy to decide what is the base type for each parameter. While adding the second driver (Zurich or Qblox), I'd enforce the default to be the same per parameter (possibly reintroducing the sweeper type only internally, not in the interface, and passing around this information). An interface for an actual sweeper type can be added, but truly later on.

A possible solution for qibocal could be to adjust the parameters to have ones for each qubit.

I'm glad that there is room to improve even the Qibocal interface, keeping it optimal and still compatible. However, which will be the exact best Qibocal interface is a separate discussion. I would open an issue in Qibocal to discuss, as soon as we stabilize this (the attempt was useful to justify the Qibolab choice, but I'd not go any further).

stavros11 commented 2 weeks ago

Thank you for the feedback. From what I understand we all agree to drop SweeperType, so I will soon open a PR that does this.

I am still not sure what is the expected behavior though. Originally the suggestion was to convert everything to ABSOLUTE, however if I understand correctly @alecandido is now proposing to follow the behavior defined by the QM driver, which is:

As far as I know, this is also how sweepers are currently used in qibocal, so in practice it is convenient to leave as they are (it will also be less work on the driver). I also agree that some of these options, such as OFFSET for frequency, are more intuitive, except maybe amplitude which could also be more convenient as ABSOLUTE (I can change that, if you agree). That being said, I see that it would be confusing for users that different parameters have different behavior, even if we document it.

Also, not related to sweeper types, but shall I drop attenuation, gain and lo_frequency as it is unlikely that there are instruments that provide real-time sweepers of these? And at this stage, also rename bias to offset?

andrea-pasquale commented 2 weeks ago

We could really convert it to absolute, but then Qibolab would only deal with ~GHz frequencies, and the conversion should be done inside Qibocal, fishing the base value from wherever it is.

I would be fine with doing the conversion in Qibocal. Especially if you consider non-runcard users this is already quite trivial to do, since we can just query the frequency in the python script. This change would be minimal compare to the changes that you already performing to support 0.2

  • frequency -> OFFSET

  • amplitude -> FACTOR (note that QM has the following limitation: -2 < factor < 2)

  • duration -> ABSOLUTE

  • duration_interpolated -> ABSOLUTE

  • relative_phase -> ABSOLUTE

  • attenuation -> not available

  • gain -> not available

  • bias -> OFFSET

  • lo_frequency -> not available

I would say again that we should just have absolute values. When calibrating two qubit gates having amplitude sweeps done using factor can be painful.

Also, not related to sweeper types, but shall I drop attenuation, gain and lo_frequency as it is unlikely that there are instruments that provide real-time sweepers of these?

We have some protocols in qibocal which are sweeping attenuation (for qblox) but I would hope that we can get rid of them soon-ish. I'm also happy to drop both gain and lo_frequency.

And at this stage, also rename bias to offset?

Yes, makes sense.

alecandido commented 2 weeks ago

I would say again that we should just have absolute values. When calibrating two qubit gates having amplitude sweeps done using factor can be painful.

If for Qibocal is better to have all of them as absolute, then let's do so. Even for Qibolab it's the easiest (just setting some values, no operation).

We have some protocols in qibocal which are sweeping attenuation (for qblox) but I would hope that we can get rid of them soon-ish. I'm also happy to drop both gain and lo_frequency.

Regarding this, there might be some value in having them as sweepers, even software ones, but on the device (if available). It would save some networking. However, I'm not aware that this is possible. @hay-k confirmed that near-time Zurich sweepers are actually laptop-time sweepers (in this case zeraa-time sweepers, i.e. the same machine running Qibolab and Qibocal). So, for the time being, I'd just drop them, and keep the loops in Qibocal.

Yes, makes sense.

Both are fine for me. Choose the most consistent one, according to your own taste.

stavros11 commented 2 weeks ago

Regarding this, there might be some value in having them as sweepers, even software ones, but on the device (if available). It would save some networking. However, I'm not aware that this is possible. @hay-k confirmed that near-time Zurich sweepers are actually laptop-time sweepers (in this case zeraa-time sweepers, i.e. the same machine running Qibolab and Qibocal). So, for the time being, I'd just drop them, and keep the loops in Qibocal.

The main issue with gain and attenuation is that they don't appear anywhere in the existing pulses or configs. Of course, after #995 qblox may provide it's own custom configs that have an attenuation or gain, but it's a bit weird that the Sweeper (a general interface object) has information about custom configs.

I am also not sure to what extent we can sweep them even on device (even if not real time). I think QM provides a way to do so, but I have never tried it.

Both are fine for me. Choose the most consistent one, according to your own taste.

Same as the above, none of the existing configs has a bias, but the DcConfig has an offset, so I'd go with that.

alecandido commented 2 weeks ago

I am also not sure to what extent we can sweep them even on device (even if not real time). I think QM provides a way to do so, but I have never tried it.

The OPX and the Octave have an API for setting (the set_* methods) https://docs.quantum-machines.co/1.2.0/docs/API_references/qm_api/#qm.quantum_machine.QuantumMachine.set_digital_buffer https://docs.quantum-machines.co/1.2.0/docs/API_references/qm_octave/#qm.octave.qm_octave.QmOctaveBase.set_clock But they are not part of QUA, so I'm quite convinced they can not be made part of a qm.qua.Program, and thus not modified on the device.

If you have a different idea, it would be quite interesting. But it's an optimization anyhow, so let's keep it simple, and leave the Python loops to Qibocal

Same as the above, none of the existing configs has a bias, but the DcConfig has an offset, so I'd go with that.

Ah, right. So yes, I perfectly agree it should be offset, not bias (or they should be the same - but it's more practical to touch the sweeper at this point).

stavros11 commented 1 week ago