Kernel class #758

Jacfomg · 2024-01-16T07:28:05Z

Follows what discussed in #754. The only thing I removed the kernel from the qubit repr

Checklist:

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

codecov · 2024-01-16T07:31:27Z

Codecov Report

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

Comparison is base (3063105) 63.86% compared to head (8f49f89) 63.97%.

Files	Patch %	Lines
src/qibolab/instruments/zhinst.py	66.66%	2 Missing ⚠️
src/qibolab/__init__.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #758      +/-   ##
==========================================
+ Coverage   63.86%   63.97%   +0.10%     
==========================================
  Files          47       49       +2     
  Lines        5756     5773      +17     
==========================================
+ Hits         3676     3693      +17     
  Misses       2080     2080

Flag	Coverage Δ
unittests	`63.97% <94.00%> (+0.10%)`	⬆️

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.

src/qibolab/dummy.py

src/qibolab/kernels.py

src/qibolab/dummy.py

src/qibolab/kernels.py

src/qibolab/serialize.py

tests/dummy_qrc/zurich.py

andrea-pasquale

Thanks @Jacfomg.
Apart from some minor changes this PR should be ready to merge.

src/qibolab/instruments/zhinst.py

src/qibolab/kernels.py

andrea-pasquale · 2024-01-17T06:07:30Z

src/qibolab/serialize.py

-        single_qubit = runcard["characterization"]["single_qubit"]
-        for qubit in qubits.values():
-            qubit.kernel_path = extras_folder / single_qubit[qubit.name]["kernel_path"]
+        kernels = Kernels.load(path=extras_folder / "kernels.npz")


I would define "kernels.npz" above a top level variable.

When using it I found here that I would like

Kernels(kernels).dump(Path(__file__).parent / "dummy" / KERNELS_FILE)

to be something like

Kernels(kernels).dump(Path(__file__).parent / platform.name / KERNELS_FILE)

but for some reason we it can be named "dummy" or "dummy_couplers" should we join them ?

src/qibolab/serialize.py

src/qibolab/dummy.py

stavros11 · 2024-01-19T14:30:52Z

src/qibolab/serialize.py

-        single_qubit = runcard["characterization"]["single_qubit"]
-        for qubit in qubits.values():
-            qubit.kernel_path = extras_folder / single_qubit[qubit.name]["kernel_path"]
+        kernels = Kernels.load(path=extras_folder / KERNELS_FILE)


At this stage extras_folder is used only for the kernels. Why don't we accept the whole kernel path (including the file name) instead of hardcoding to "kernels.npz"? Maybe I want to use a different name for my kernel file, or play with different kernel files for the same platform.

I guess it's really a matter of taste.

But you can do the things you asked even with the current strategy. Just have multiple kernel files inside the extras_folder, and rename those to kernels.npz.

About the idea of replacing a folder with a file, since there is nothing else, in general I'd agree with you. But here it would be already better to have a single file per runcard (to avoid spreading them), and we start from two (the .py and .yml).

Maybe I have a counterproposal: what if we keep the directory, but we move all the files in there?

I.e.

iqm5q/ platform.py parameters.yml kernels.npz

At least it would be easy to collect all the related files, and in case you want to preserve/circulate a platform, you could always tar/zip the folder and grab it as a single file.

It seemed more future proof as we may need more stuff to be in that folder, and also what @alecandido mentioned would be more organized so we can do it on another PR on the qrc_cluster platforms.

iqm5q/ platform.py parameters.yml kernels.npz

In my opinion, this is the best solution, but I agree doing it in another PR because it requires a few more changes which are unrelated to the kernels.

@alecandido @stavros11 @Jacfomg if everyone agrees I can open a PR in qibocal to generate directly folders and not just platform runcards. I believe that it should be feasible. It should be useful for the qibocal user to be able to see directly the create_function alongside the parameters and the kernels.

Indeed, it would be useful. I would just wait to do it after we implement the structure proposed above (one directory per platform) in qibolab and in the platforms. Also, I think it makes more sense to implement the function you are proposing in qibolab, perhaps replacing the current dump_runcard and just use it qibocal, because at the end of the day it solves the problem of platform serialization which is a qibolab problem. However, this is a bit orthogonal to #762, so let’s wait to see what others think.

You are right. The function should be in qibolab. I will wait.

I agree with you that we should try to roll it out, and we should try to do it in Qibolab.
Btw, if it's going through Qibolab it should be also a non-breaking change, since Qibocal should not care much about the runcard serialization structure.

However, this is a bit orthogonal to #762, so let’s wait to see what others think.

I would say orthogonal is the correct word, not opposite. The moment we implement the folder, we could well dump the parameters in JSON, and drop the PyYAML dependency as well.

src/qibolab/serialize.py

stavros11 · 2024-01-19T14:42:46Z

src/qibolab/serialize.py

+        if qubit.kernel is not None:
+            kernels[qubit.name] = qubit.kernel
+            qubit.kernel = None
+    Kernels(kernels).dump(Path(__file__).parent / "dummy" / KERNELS_FILE)


I think this method is used by all platforms. Why are we always dumping in the directory of the dummy?

I think this point requires some discussion. If you want to always dump the kernels in the file you loaded them from, it is probably convenient to make path an attribute of Kernels instead of passing it seperately to load and dump. But I am not sure if we always want to overwrite kernels during calibration. If no, we need a different mechanism to pass the the path to dump to, which will also be exposed to qibocal (and the user).

Two reasons, the first I forgot and the second it's related to this. #758 (comment) I wanted to use the platform.name but the dummy has two, I think the best would be something like this ?

name = platform.name if platform.name == "dummy_couplers": name = "dummy" Kernels(kernels).dump(Path(__file__).parent / name / KERNELS_FILE)

For as long as we need to maintain two dummy platforms, couldn't we have actually two folders?

I am not that much concerned about the dummy, the implemented solution should be fine. However, this is still a bit problematic because it is asymmetric to the loading.

If I understand it correctly, when you create the platform you will load the kernels from whatever extras_folder is passed to load_qubits. This is typically located together with the platform runcard. Then when you (or qibocal) call dump it is using Path(__file__).parent / name to determine the directory, where __file__ is the serialize.py. Therefore it will dump the new kernels inside your qibolab installation, which is in general a different place than where the runcards are stored. Is this what we want?

More info: I understand this works okay for the dummy, but did you try it with other platforms, eg. iqm5q?

That's the other solution, I'm fine with any.

We want that for the dummy, but not for the other platforms. I will think of something then. We would need the extras_folder or something in qibocal as well to dump the kernels properly, any suggestions ?

I think dump_runcard is automatically called by qibocal after every routine. So if you add kernels to any platform (eg. iqm5q) and you run any qibocal routine, after execution it will try to dump the "new" kernels to a directory like

qibolab/src/qibolab/iqm5q/kernels.npz

and will probably fail if there is no folder iqm5q within qibolab. At least that's what I think, I didn't do the actual test.

Tests are failing because of that, can I add the extras_folder path as a atribute for the platform ?

That’s probably acceptable for now and we can improve once we implement the folder structure proposed by @alecandido.

I agree we could go incrementally (eventually we can even use an env var, or even better the same variable we use to locate the runcards).

qibolab/src/qibolab/__init__.py

Line 21 in 3416a8e

def get_platforms_path():

However, for dummy we should mock the serialization, even when deployed: the installation folder might require privileges that the user does not have.
Furthermore, I start considering the option to split dummy from the regular Qibolab (we could not implement just in tests/ because it's being used also by Qibocal... I'm thinking...)

Outdated

alecandido · 2024-01-19T16:58:29Z

@Jacfomg could you also merge/rebase on main?

alecandido · 2024-01-19T17:39:18Z

I definitely agree with this @stavros11 here #758 (comment), and it's sensible to address also the other comments.

But concerning the rest, I have nothing else to amend. Once solved and rebased, you could merge (I could give a further approval, but since I'd wait for the comments above, I'll leave the pleasure to @stavros11).

Currenlty working on this PR

andrea-pasquale · 2024-01-24T15:33:13Z

@stavros11 I implemented the folders approach discussed here:

iqm5q/
  platform.py
  parameters.yml
  kernels.npz

If you approve it I will open the corresponding PR in qibolab_platforms_qrc.

alecandido · 2024-01-24T15:53:47Z

If you approve it I will open the corresponding PR in qibolab_platforms_qrc.

First we'd need to have a PR for that in Qibolab ;)

(you could start from there if you wish)

Internal names are up for discussion...

alecandido · 2024-01-24T15:56:33Z

Btw, as @stavros11 pointed out above, this is orthogonal to #762 in a certain sense, since they are two separate changes.
But on the other side, they are acting on the same surface (since they are both concerning the serialization format). So, for whoever is going to author the Qibolab PR, I'd include the conversion to JSON, since it's a tiny change.

andrea-pasquale · 2024-01-24T16:07:47Z

If you approve it I will open the corresponding PR in qibolab_platforms_qrc.

First we'd need to have a PR for that in Qibolab ;)

(you could start from there if you wish)

Internal names are up for discussion...

I've just implemented it here in this PR :)

alecandido · 2024-01-24T16:17:03Z

Ah, I didn't notice 0351323

alecandido · 2024-01-24T16:20:39Z

@andrea-pasquale I would say names are up for discussion, but I'd avoid using the folder name for internal names. If we choose something, you won't need to know anything beyond the folder: after that, all we be loaded in the same way.

I know it's now a minimal difference, because the only function resolving the internal names is the same one receiving the path. But it's more decoupled if you do not require them to be the same (e.g. you could just rename the folder, without renaming any file inside).

andrea-pasquale · 2024-01-24T18:01:27Z

@andrea-pasquale I would say names are up for discussion, but I'd avoid using the folder name for internal names. If we choose something, you won't need to know anything beyond the folder: after that, all we be loaded in the same way.

I know it's now a minimal difference, because the only function resolving the internal names is the same one receiving the path. But it's more decoupled if you do not require them to be the same (e.g. you could just rename the folder, without renaming any file inside).

If I understood correctly since all that matters is the path to the folder every file inside the folder can be standardized, correct?
Which is actually what you were suggesting in your original approach...

iqm5q/
  platform.py
  parameters.yml
  kernels.npz

Since I was just moving files around I forgot to think about using standardized names.

alecandido · 2024-01-24T18:39:09Z

Indeed.

I wanted to point out that I'm not particularly keen on those names, for as long as we pick some standardized names.

andrea-pasquale · 2024-01-24T18:40:54Z

Indeed.

I wanted to point out that I'm not particularly keen on those names, for as long as we pick some standardized names.

Great, so I understood correctly.
I am also not good at names so I used exactly the same ones that you proposed in 853dc97

andrea-pasquale · 2024-01-24T19:00:21Z

Btw, as @stavros11 pointed out above, this is orthogonal to #762 in a certain sense, since they are two separate changes.
But on the other side, they are acting on the same surface (since they are both concerning the serialization format). So, for whoever is going to author the Qibolab PR, I'd include the conversion to JSON, since it's a tiny change

I didn't really follow the discussion related to #762. The plan is to switch from yaml to json everywhere?

alecandido · 2024-01-24T19:16:41Z

I didn't really follow the discussion related to #762. The plan is to switch from yaml to json everywhere?

In Qibolab YAML is used in a single place, a runcard that should not be manually written.
Plus, it is an external dependency. A further one, and non-negligible (though not the most controversial one).

json is there in any case, and for the time being we don't need anything else.

A few notes in advance: for when you need to write manually, it's not difficult to convert to YAML and convert back:

from pathlib import Path
import json
import sys

import yaml

path = Path(sys.argv[1])
d = json.loads(path.load_text())
path.write_text(yaml.dumps(d))

and forth (swap json and yaml)

We could make a minimal CLI in a separate package (distributed together, with dependencies as extras - or even distributed separately, or not at all, if it's simpler and enough), named qibolab to help with chores like these (if ever relevant), and e.g. to bootstrap a platform folder from a very basic description (e.g. the number of qubits, or not even that).

Co-authored-by: Alessandro Candido <[email protected]>

for more information, see https://pre-commit.ci

Partial port from #782

alecandido · 2024-01-25T15:44:27Z

src/qibolab/serialize.py

+def dump_kernels(platform: Platform, path: Path):
+    """Creates Kernels instance from platform and dumps as npz.
+
+    Args:
+        platform (qibolab.platform.Platform): The platform to be serialized.
+        path (pathlib.Path): Path that the kernels file will be saved.
+    """
+
+    # create and dump kernels
+    kernels = Kernels()
+    for qubit in platform.qubits.values():
+        if qubit.kernel is not None:
+            kernels[qubit.name] = qubit.kernel
+
+    kernels.dump(path / KERNELS)


@stavros11 @hay-k

For even better isolation, eventually I'd consider having these functions just writing on byte streams, and returning them.
The top level dump could be the one responsible to attribute each byte stream a path, and dump it there.

In any case, it's not for this PR.

The drawback of byte streams is just memory consumption: if you dump on streams, instead of directly on disk, you double the amount of memory you consume.

But I'd say CPU memory should not be an issue in Qibolab...

andrea-pasquale

Thanks for isolating the conversion to JSON in #782 @alecandido

stavros11

Thanks @andrea-pasquale. Looks good to me, other than some minor comments.

Even though it already has two approvals, the only thing before merging is that I’d open the corresponding PR in qibolab_platforms_qrc and try to test with some instruments (which I believe we haven’t done yet). Let me know if you’ll do that, otherwise I can also help rewriting the existing platforms to the new folder structure.

stavros11 · 2024-01-26T06:10:56Z

doc/source/getting-started/experiment.rst

+
+where ``platform.py`` contains instruments information, ``parameters.yml``
+includes calibration parameters and ``kernels.npz`` is an optional
+file with additional calibration parameters.


In principle, parameters.yml is also optional, as someone can define the qubit/channel objects hardcoded in the create. This is basically explained in the lab.rst tutorial.

However, dumping the platform after will automatically create parameters, so I am not sure whether we should advocate this kind of usage.

doc/source/tutorials/lab.rst

src/qibolab/serialize.py

stavros11 · 2024-01-26T06:31:47Z

src/qibolab/dummy/__init__.py

We could expose create_dummy here to keep the same interface as before

from qibolab.dummy import create_dummy

stavros11 · 2024-01-26T06:34:29Z

src/qibolab/__init__.py

@@ -29,32 +30,33 @@ def get_platforms_path():
    return Path(profiles)


-def create_platform(name, runcard=None):
+def create_platform(name, path: Path = None) -> Platform:


I have the impression that the path argument is no longer required here. Before it was used to link a YAML to the create (assuming you wanted to use different YAMLs for the same create?). But this is no longer valid given that we’re proposing a fixed folder structure.

However, this will become more clear once we try to define some actual platforms (eg in qibolab_platforms_qrc).

stavros11 · 2024-01-26T06:44:29Z

src/qibolab/serialize.py

+        platform (qibolab.platform.Platform): The platform to be serialized.
+        path (pathlib.Path): Path where yaml and npz will be dumped.
+    """
+


Didn’t try to use this yet, but shouldn’t it:

Create the folder specified by path if it doesn’t exist (given that it’ll try to write files inside)

Dump also platform.py inside this folder?

I’ll try to use and review this comment.

Co-authored-by: Stavros Efthymiou <[email protected]>

andrea-pasquale · 2024-01-26T07:49:07Z

Even though it already has two approvals, the only thing before merging is that I’d open the corresponding PR in qibolab_platforms_qrc and try to test with some instruments (which I believe we haven’t done yet). Let me know if you’ll do that, otherwise I can also help rewriting the existing platforms to the new folder structure.

Thanks for the review @stavros11.
I've just opened the PR this morning qiboteam/qibolab_platforms_qrc#108

stavros11 · 2024-01-26T09:38:13Z

src/qibolab/serialize.py

+        if qubit.kernel is not None:
+            kernels[qubit.name] = qubit.kernel
+
+    kernels.dump(path / KERNELS)


I think this will not work. KERNELS is appended to the path twice, once here and once in

qibolab/src/qibolab/kernels.py

Line 37 in 5e46e6f

path / KERNELS,

We should test the PRs better before considering to merge. Particularly the new dump_kernels and dump_platform methods, I believe are not captured by the tests, while they can be easily tested without access to instruments/QPUs.

Yes, I also found the same error while trying to run tests in qibocal.
I will try to add tests to cover dump_platform and dump_kernels.

Tests added in 8f49f89

stavros11 · 2024-01-26T13:43:30Z

Thanks @andrea-pasquale. I will merge this to unlock the CI in the platforms repo (and the change is already big). If you like to change dump_platform to run a folder please open a new PR (I'll open an issue).

Jacfomg requested review from alecandido, stavros11 and andrea-pasquale January 16, 2024 07:28

Jacfomg mentioned this pull request Jan 16, 2024

Add kernel paths for qubits in dummy.yml #752

Closed

4 tasks

andrea-pasquale reviewed Jan 16, 2024

View reviewed changes

src/qibolab/dummy.py Outdated Show resolved Hide resolved

src/qibolab/kernels.py Outdated Show resolved Hide resolved

alecandido reviewed Jan 16, 2024

View reviewed changes

src/qibolab/dummy.py Outdated Show resolved Hide resolved

src/qibolab/kernels.py Outdated Show resolved Hide resolved

alecandido previously approved these changes Jan 16, 2024

View reviewed changes

andrea-pasquale reviewed Jan 17, 2024

View reviewed changes

andrea-pasquale previously approved these changes Jan 19, 2024

View reviewed changes

stavros11 reviewed Jan 19, 2024

View reviewed changes

alecandido self-requested a review January 19, 2024 16:57

alecandido removed their request for review January 19, 2024 17:39

Jacfomg requested a review from stavros11 January 19, 2024 17:47

andrea-pasquale requested a review from alecandido January 25, 2024 07:07

Jacfomg and others added 12 commits January 25, 2024 16:26

Update src/qibolab/kernels.py

0f3ca6d

Co-authored-by: Alessandro Candido <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

7f7bee9

for more information, see https://pre-commit.ci

fix: Refactor kernel saving and loading

e12a93e

fix: path with platform name for Kernels

b0b24f9

refactor: Introduce folders for platform serialization

9f37fd8

fix: Fix test in doc

1e6a7dc

refactor: Standardize file names

3636038

fix: Fix doctest

4b8a505

fix: Fix tests related to qibosoq 0.1.1

55723ed

doc: Improve documentation following new folder structure

26e30d8

doc: Update QIBOLAB_PLATFORMS meaning

d33823c

fix: Usage of runcard folders in docs

fa13c5d

Partial port from #782

alecandido force-pushed the kernel branch from e37ff17 to fa13c5d Compare January 25, 2024 15:26

fix: Postpone more json occurrences

d869de0

alecandido approved these changes Jan 25, 2024

View reviewed changes

andrea-pasquale approved these changes Jan 25, 2024

View reviewed changes

stavros11 reviewed Jan 26, 2024

View reviewed changes

andrea-pasquale mentioned this pull request Jan 26, 2024

New folder layout qiboteam/qibolab_platforms_qrc#108

Merged

Update doc/source/tutorials/lab.rst

c13a8a7

Co-authored-by: Stavros Efthymiou <[email protected]>

fix: Fix dummy interface

5e46e6f

stavros11 reviewed Jan 26, 2024

View reviewed changes

andrea-pasquale added 2 commits January 26, 2024 15:00

refactor: Dump kernels only if they are not empty

a3fd68f

test: Implement additional tests dumping and loading

8f49f89

stavros11 merged commit b861009 into main Jan 26, 2024
24 checks passed

stavros11 deleted the kernel branch January 26, 2024 13:43

andrea-pasquale mentioned this pull request Jan 26, 2024

Updating mechanism to dump platforms qiboteam/qibocal#698

Merged

8 tasks

andrea-pasquale mentioned this pull request Feb 9, 2024

Replace kernel_path with kernel #754

Closed

Kernel class #758

Kernel class #758

Conversation

Jacfomg commented Jan 16, 2024 • edited by andrea-pasquale Loading

codecov bot commented Jan 16, 2024 • edited Loading

Codecov Report

andrea-pasquale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stavros11 Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stavros11 Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacfomg Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

stavros11 Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecandido commented Jan 19, 2024

alecandido commented Jan 19, 2024

andrea-pasquale commented Jan 24, 2024

alecandido commented Jan 24, 2024 • edited Loading

alecandido commented Jan 24, 2024

andrea-pasquale commented Jan 24, 2024 • edited Loading

alecandido commented Jan 24, 2024

alecandido commented Jan 24, 2024

andrea-pasquale commented Jan 24, 2024

alecandido commented Jan 24, 2024

andrea-pasquale commented Jan 24, 2024

andrea-pasquale commented Jan 24, 2024

alecandido commented Jan 24, 2024 • edited Loading

alecandido Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-pasquale left a comment

Choose a reason for hiding this comment

stavros11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-pasquale commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stavros11 commented Jan 26, 2024

Jacfomg commented Jan 16, 2024 •

edited by andrea-pasquale

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading

alecandido Jan 19, 2024 •

edited

Loading

stavros11 Jan 22, 2024 •

edited

Loading

stavros11 Jan 19, 2024 •

edited

Loading

Jacfomg Jan 19, 2024 •

edited

Loading

stavros11 Jan 19, 2024 •

edited

Loading

alecandido commented Jan 24, 2024 •

edited

Loading

andrea-pasquale commented Jan 24, 2024 •

edited

Loading

alecandido commented Jan 24, 2024 •

edited

Loading

alecandido Jan 25, 2024 •

edited

Loading