qiboteam / qibolab

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

Kernel class #758

Closed Jacfomg closed 7 months ago

Jacfomg commented 7 months ago

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

Checklist:

codecov[bot] commented 7 months ago

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 :warning:
src/qibolab/__init__.py 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/qiboteam/qibolab/pull/758/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibolab/pull/758/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `63.97% <94.00%> (+0.10%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alecandido commented 7 months ago

@Jacfomg could you also merge/rebase on main?

alecandido commented 7 months ago

I definitely agree with this @stavros11 here https://github.com/qiboteam/qibolab/pull/758#discussion_r1459128675, 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).

andrea-pasquale commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

Ah, I didn't notice 0351323eb05b8f712ee046bd192d19d3c7956316

alecandido commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 https://github.com/qiboteam/qibolab/pull/758/commits/853dc9793787d8c77d2cc650d4acdf3376cdd5f2

andrea-pasquale commented 7 months ago

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 commented 7 months ago

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).

andrea-pasquale commented 7 months ago

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).

Indeed I was planning to wrote a script similar to the one that you provided. Thanks! I converted everything to json and I dropped the PYAML dep.

andrea-pasquale commented 7 months ago

After changing a bit the documentation I realized that also QIBOLAB_PLATFORMS should have a different meaning now. Before it was seen as "path to the create file" while now it should just be a folder that contains platforms (folders), correct?

andrea-pasquale commented 7 months ago

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 https://github.com/qiboteam/qibolab_platforms_qrc/pull/108

stavros11 commented 7 months ago

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).