Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor after update to qibolab 0.2 #1095

Open
8 tasks
andrea-pasquale opened this issue Feb 19, 2025 · 4 comments
Open
8 tasks

Refactor after update to qibolab 0.2 #1095

andrea-pasquale opened this issue Feb 19, 2025 · 4 comments

Comments

@andrea-pasquale
Copy link
Contributor

Now that we have recently completed the update to Qibolab 0.2 in #990. We can proceed with a refactor aimed at simplifying the code and reducing code duplication. Here is a first tentative list of features that we would like to implement:

  • Pass backend to acquisition function. This should solve some of the problems originally addressed in Handling global backend for protocols involving circuits #1076 which proposed as a solution the creation of a local backend inside the acquisition function itself. If we could pass directly the backend to the acquisition function there should be no need to recreate it once again.
  • Connected to Data handling and record arrays #1053 we would like to remove register_qubit and find an easier way to process the data retrieved from qibolab. A possible solution suggested by @hay-k consists in mapping each call to platform.execute() to a dedicated attribute inside the Data class which could be later stored as an array. Any post-processing on the data should be done by external function (not class methods), in order to easily test them. For storing the Data class itself we could try to come up with a mechanism to store together all these different arrays or for time being, given that we are not dealing with huge data just store each one in a separated file.
  • Both acquisition and fit function for each protocol should tackle only one qubit/pair at a time. In this way we should be able to remove most of the for loops going on in both the acquisition and the fitting. We could consider to have an Acquisition object (target one qubit or pair)
from qibolab import PulseSequence, Sweeper
from qibolab._core.execution_parameters import ExecutionParameters
from dataclasses import dataclass

@dataclass
class Acquisition:
    sequence: PulseSequence
    sweepers: list[Sweepers]
    parameters: ExecutionParameters

Given two Acquisition objects Qibocal could try to perform a parallel execution and throw an error if it is not possible. The user should be able to specify if the execution should be in parallel for all targets.

@alecandido
Copy link
Member

  • Pass backend to acquisition function. This should solve some of the problems originally addressed in Handling global backend for protocols involving circuits #1076 which proposed as a solution the creation of a local backend inside the acquisition function itself. If we could pass directly the backend to the acquisition function there should be no need to recreate it once again.

Instead of this, I would also consider the more ambitious option of not executing anything at all in _acquisition, and go deeper even in the direction of your second point

  • possible solution suggested by @hay-k consists in mapping each call to platform.execute() to a dedicated attribute inside the Data class

I.e. do not make calls in to Platform.execute(), but rather return the execution input.

E.g., if you allow just a sequence of calls (which will happen sequentially in any case, so I'd recommend it, even following the discussion in #1089), you may just return from _acquisition a list of inputs (possibly dictionaries), with a couple of layouts:

  • one for Platform.execute()
  • one for Backend.execute_circuit()

Fundamentally, it means to define acquisition with the following type:

acquisition: Callable[[_ParametersT], Union[Sequence[PulseInput], Sequence[CircuitInput]]]

or even to use two different attributes (e.g. acquisition_pulse and acquisition_circuit), and make them mutually exclusive.

Once that is done, the results should just be collected in a single array, with the usual dimension +1, and the additional one being the sequence of experiments itself (if multiple experiments are needed).
Now, if there is a case in which experiments with inhomogeneous results are needed, you may consider the sequence of arrays (possibly using again .npz). But I'd rather suggest splitting these kinds of acquisitions over multiple routines, and handle them at the level of a script encompassing multiple experiments, which would be more modular and reusable (rather than stuffing everything in a single very specific routine).

  • Both acquisition and fit function for each protocol should tackle only one qubit/pair at a time. In this way we should be able to remove most of the for loops going on in both the acquisition and the fitting. We could consider to have an Acquisition object (target one qubit or pair)

I agree about this, and, in my opinion, it would fit nicely in the above plan.

@alecandido
Copy link
Member

alecandido commented Feb 20, 2025

  • Avoid disconnection when running circuits (this is mainly a Qibolab feature request) to avoid wasting time connecting and reconnecting for protocols like the RB.

Btw, I believe we may do better, and possibly expose direct access to the compiler. Rather than forcing Qibocal to pass through the same mechanism intended for individual runs (i.e. the QibolabBackend). Though it is convenient to use a Qibo-compliant backend, to restore the parallel with the simulation backends, and simplify their integration (possibly, we could then add a toggle to the backends' configuration to just avoid disconnections).

However, for individual protocols (like the RB) the feature is already there, i.e. QibolabBackend.execute_circuits() (instead of its singular counterpart). If the protocol is long enough, the connection/disconnection time will be negligible (not sure what enough means exactly, but I expect ~1s).

@andrea-pasquale
Copy link
Contributor Author

Thanks for the feedback @alecandido.

  • Pass backend to acquisition function. This should solve some of the problems originally addressed in Handling global backend for protocols involving circuits #1076 which proposed as a solution the creation of a local backend inside the acquisition function itself. If we could pass directly the backend to the acquisition function there should be no need to recreate it once again.

Instead of this, I would also consider the more ambitious option of not executing anything at all in _acquisition, and go deeper even in the direction of your second point

  • possible solution suggested by @hay-k consists in mapping each call to platform.execute() to a dedicated attribute inside the Data class

I.e. do not make calls in to Platform.execute(), but rather return the execution input.

E.g., if you allow just a sequence of calls (which will happen sequentially in any case, so I'd recommend it, even following the discussion in #1089), you may just return from _acquisition a list of inputs (possibly dictionaries), with a couple of layouts:

* one for `Platform.execute()`

* one for `Backend.execute_circuit()`

Fundamentally, it means to define acquisition with the following type:

acquisition: Callable[[_ParametersT], Union[Sequence[PulseInput], Sequence[CircuitInput]]]

or even to use two different attributes (e.g. acquisition_pulse and acquisition_circuit), and make them mutually exclusive.

Once that is done, the results should just be collected in a single array, with the usual dimension +1, and the additional one being the sequence of experiments itself (if multiple experiments are needed). Now, if there is a case in which experiments with inhomogeneous results are needed, you may consider the sequence of arrays (possibly using again .npz). But I'd rather suggest splitting these kinds of acquisitions over multiple routines, and handle them at the level of a script encompassing multiple experiments, which would be more modular and reusable (rather than stuffing everything in a single very specific routine).

  • Both acquisition and fit function for each protocol should tackle only one qubit/pair at a time. In this way we should be able to remove most of the for loops going on in both the acquisition and the fitting. We could consider to have an Acquisition object (target one qubit or pair)

I agree about this, and, in my opinion, it would fit nicely in the above plan.

I see your point. I mentioned to include the Backend before splitting the acquisition into multiple qubits because I believe that it should be easier to implement. Regarding lifting the execution outside the acquisition we need to check if this can be done for all protocols, given that there might be some non-trivial post-processing required to store the data. This is why I would tend to keep the execution inside the acquisition function at least during a first refactoring.

Btw, I believe we may do better, and possibly expose direct access to the compiler. Rather than forcing Qibocal to pass through the same mechanism intended for individual runs (i.e. the QibolabBackend). Though it is convenient to use a Qibo-compliant backend, to restore the parallel with the simulation backends, and simplify their integration (possibly, we could then add a toggle to the backends' configuration to just avoid disconnections).

However, for individual protocols (like the RB) the feature is already there, i.e. QibolabBackend.execute_circuits() (instead of its singular counterpart). If the protocol is long enough, the connection/disconnection time will be negligible (not sure what enough means exactly, but I expect ~1s).

Regarding this I know that execute_circuits could be a solution. However, my point was mainly related to the possibility of having this expert mode offered by Qibolab where the disconnection doesn't happen after each circuit execution. This could potentially be an issue if after a protocol involving circuits we need to run simpler experiments with pulse sequences where we would need to reconnect manually (I think that before we didn't see this because of the weird behavior with global backends but it might be worth to investigate).

@alecandido
Copy link
Member

alecandido commented Feb 20, 2025

Regarding lifting the execution outside the acquisition we need to check if this can be done for all protocols, given that there might be some non-trivial post-processing required to store the data. This is why I would tend to keep the execution inside the acquisition function at least during a first refactoring.

Agreed. But it is more about how the acquisition is realized, since the result layout is quite standard because of Qibolab.
In any case, it's not urgent, and we can discuss how to do it exactly. Just keep it in mind while you're implementing the first iteration, as you may already structure the refactored _acquisition to be sufficiently friendly for the second step.

Regarding this I know that execute_circuits could be a solution. However, my point was mainly related to the possibility of having this expert mode offered by Qibolab where the disconnection doesn't happen after each circuit execution. This could potentially be an issue if after a protocol involving circuits we need to run simpler experiments with pulse sequences where we would need to reconnect manually (I think that before we didn't see this because of the weird behavior with global backends but it might be worth to investigate).

Agreed, let's provide better support on the Qibolab side. Which more or less boils down to stabilize the compiler, since what is being done is [...] which is pretty cheap to reproduce on the Qibocal side. We just need to guarantee that the Compiler won't change right after.

EDIT: ok, the one above was the wrong proposal - it will make it harder to use simulation; no, we should just put the platform.connect()/.disconnect() toggles behind a flag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants