qiboteam / qibolab

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

Replace `kernel_path` with `kernel` #754

Closed andrea-pasquale closed 7 months ago

andrea-pasquale commented 7 months ago

Currently in order to store kernels there is a kernel_path attribute in Qubit. I think that a better interface choice would be to be able to get the kernels directly from Qubit using a kernel attribute. In order to do this we could define a separated object called Kernel or Kernels. Here is a possible example:

from dataclasses import dataclass
from pathlib import Path
from qibolab.qubits import QubitId
import numpy as np

@dataclass
class Kernel:

    data: np.array 

    @classmethod
    def load(cls, path: Path):
        return cls(data=dict(np.load(path)))

    def dump(self, path: Path):
        np.save(self.data, path)

    def __getitem__(self, qubit: QubitId):
        return self.data[qubit]

In the create_function in https://github.com/qiboteam/qibolab_platforms_qrc/ we can load kernels and then assign the corresponding one to each qubit using something like this:

kernel = Kernel.load(PATH)
qubits, couplers, pairs = load_qubits(runcard)
for q in qubits:
    qubits[q] = kernel[q]

One would also need to perform the dumping of the kernel so that Qibocal can call a generic dumping method in qibolab that will completely serialize the platform (including kernels).

The main reason behind this proposal is that in https://github.com/qiboteam/qibocal/pull/582 during the update method qibocal is directly storing the kernels to file https://github.com/qiboteam/qibocal/blob/9f1db1f0a903c43dd6514e612d8969bfd92c23fa/src/qibocal/update.py#L227-L231 while I think that a more suitable approach would be to just assign the corresponding attribute to Qubit and then delegate the dumping to qibolab. Thoughts @stavros11 @AleCandido @hay-k @Jacfomg ?

alecandido commented 7 months ago

I agree with @andrea-pasquale proposal. I never focused too much on the kernels, but having a path is definitely inconsistent with everything else.

Essentially, we should keep separate the actual data from their serialization. The data may exist in-memory (during execution) or on disk (before or after execution), and the in-memory part should be fully in memory (unless we will have special needs because of memory limits, but that's not the case). Switching from one representation to the other is the serialization job.

I quite agree also on the interface, but given that it has a single element, we could even decide to just make it a subclass of np.ndarray. Do we believe there will be more information related to kernels to store? (e.g. some kind of metadata)

andrea-pasquale commented 7 months ago

Thanks for the feedback @AleCandido. A np.ndarray should be enough for a single kernel. Currently each kernel is stored in a separated .npy file. We could even think about dumping all kernels in a single file (for example using np.savez) and load everything to a single Kernel class which we could renamed Kernels. This is more or less what I had in mind by adding also a __getitem__ method to the class. In this case perhaps we can avoid to perform the inheritance from np.ndarray.

alecandido commented 7 months ago

Yes, if it's Kernels instead of Kernel I agree it should not be an array, but a collection of arrays.

Again, in absence of metadata the collection would be the only attribute, so we could make it directly a list (or dict, or tuple) inheriting. But it's not needed.

Jacfomg commented 7 months ago

I would try to go with Kernels, for now only an array per qubit would be needed.

andrea-pasquale commented 7 months ago

Closed by https://github.com/qiboteam/qibolab/pull/758.