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

Platform specific Readme #187

Merged
merged 20 commits into from
Oct 31, 2024
Merged

Platform specific Readme #187

merged 20 commits into from
Oct 31, 2024

Conversation

BrunoLiegiBastonLiegi
Copy link

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi commented Oct 9, 2024

This PR adds a small script to parse the parameters.json file and generate a README.md with some basic information for each platform.
For instance, for qw5q_platinum:

qw5q_platinum

Native Gates

Single Qubit: RX, RX12, MZ

Two Qubit: CZ

Topology

Number of qubits: 5

Qubits: [0, 1, 2, 3, 4]

graph TD;
    0 <--> 2;
    1 <--> 2;
    2 <--> 3;
    2 <--> 4;
Loading

@alecandido
Copy link
Member

I like the simplicity of using Mermaid, and letting it do whatever prefers.

However, I checked the Mermaid docs, and they do not have any actual graph drawing class, so for large chips it may be doing a terrible job... (which is not a big deal right now)

For this PR, just using Mermaid is more than fine.
However, we may consider leaving an issue open to explore alternatives like Graphviz
https://www.graphviz.org/gallery/
which could also be simply integrated passing through Networkx (e.g. just loading the pairs into a networkx class, and asking it to dump the Dot file).
Other solutions are listed here https://networkx.org/documentation/stable/reference/drawing.html

But, as I said, these are ideas for later on. Right now is fine as it is, and incredibly nice in it's minimal implementation. Thanks :)

@BrunoLiegiBastonLiegi
Copy link
Author

Yeah indeed for the 11q is not working great:

graph TD;
    A1 <--> A2;
    A1 <--> A3;
    A1 <--> D5;
    A2 <--> A4;
    A2 <--> A6;
    A3 <--> A4;
    A3 <--> D4;
    A4 <--> A5;
    A4 <--> B3;
    A5 <--> B1;
    A6 <--> B3;
    A6 <--> D3;
    B1 <--> B2;
    B1 <--> B3;
    B2 <--> B4;
    B3 <--> B4;
    B4 <--> B5;
    D1 <--> D2;
    D1 <--> D3;
    D2 <--> D4;
    D3 <--> D4;
    D4 <--> D5;
Loading

I tried to look for alternative layout algorithm but I couldn't find much information...

@alecandido
Copy link
Member

Yeah indeed for the 11q is not working great:

Well, it's not ideal, but not even that bad.

It's a bit entangled, but you get all the information you need.
Maybe we can tweak it a bit, e.g. using smaller boxes around names - and in any case you can zoom and pan, which is convenient - but for even larger circuits it may be suboptimal.
In any case, this diagram could even be the default, and even replaced with a manually tuned one, which may better represent the chip features (e.g. putting them on a specific grid, and then drawing some connections - which is most likely what happens even physically on the chip, to some extent...).

@alecandido
Copy link
Member

@BrunoLiegiBastonLiegi I'd discourage you to go through the workflows' README generation, for two main reasons:

  1. the generated README may be manually edited, and you don't want to overwrite those (nor attempting generating a diff to apply, which is definitely too complex...)
  2. in order to make it work, the workflows should commit to the repo, which is not that great...

Instead, what you could do is to check in the workflow if any platform is lacking a README, and then publish a message on the PR suggesting providing such a README, and a good starting point may be achieved by running the script for that platform.

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi marked this pull request as ready for review October 10, 2024 08:03
Copy link

If you are changing the configuration of one or more platforms remember to run python generate_readme.py to update the README.md accordingly.

@BrunoLiegiBastonLiegi
Copy link
Author

Ok this should be more or less ready, the only thing I am not completely sure about concerns the native gates.
First of all, what gate is the so called RX12 that you find in the parameters.json is it the GPI2? Secondly, the RZ gate is never explicitely reported in the configuration, but I believe we always have since it's just a virtual phase, should I always add it automatically (the same applies to the Z maybe)?

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I didn't go through the whole script, and it's brief enough that is not worth optimizing that much...

But here are just a few technical and qualitative simplifications.

BrunoLiegiBastonLiegi and others added 4 commits October 10, 2024 15:20
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
@Edoardo-Pedicillo
Copy link
Contributor

Could we use this PR to add the installation requirements in the READMEs qiboteam/qibocal#988 (comment) ?

@alecandido
Copy link
Member

Could we use this PR to add the installation requirements in the READMEs qiboteam/qibocal#988 (comment) ?

Manually yes (with the script is overly complicated, for little to no advantage...).

Or even a PR on top of this one is also fine.

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last ones were just stylistic suggestions. To me, it's good to merge.

Co-authored-by: Alessandro Candido <candido.ale@gmail.com>
@stavros11 stavros11 changed the base branch from main to 0.1 October 17, 2024 16:04
@scarrazza
Copy link
Member

@BrunoLiegiBastonLiegi could you please fix the test issue?

@BrunoLiegiBastonLiegi
Copy link
Author

Not sure the error is related to this PR specifically. Even #180 was failing for the same reason.
By the way, @stavros11 @alecandido, is this not supposed to work with 0.2 due to the parameters.json being changed significantly? If that's the case, should I create the analogous script for 0.2?

@alecandido
Copy link
Member

alecandido commented Oct 30, 2024

Not sure the error is related to this PR specifically. Even #180 was failing for the same reason.

No, you're not supposed to fix that error. But the problem is not 0.2, but rather the OPX1000. The stable workflow is testing with the latest release, and the 0.1 driver for the OPX1000 is yet unreleased (in 0.1, while it has been just released for 0.2). Instead, the driver is merged in the 0.1 branch of Qibolab (the main equivalent for the 0.1 series), and that's what the dev workflow is pulling.

So, the workflows should be fine as they are.

@alecandido
Copy link
Member

alecandido commented Oct 30, 2024

By the way, @stavros11 @alecandido, is this not supposed to work with 0.2 due to the parameters.json being changed significantly? If that's the case, should I create the analogous script for 0.2?

You depend very mildly on the format of parameter.json, but still enough to break, I'm sorry...

info = {key: data[key] for key in ("nqubits", "qubits", "topology")}

none of these keys is any longer present.

Now, you should extract the qubits as the keys of the native_gates.single_qubit dictionary

"native_gates": {
"single_qubit": {
"D1": {

while the nqubits will be just len(qubits). And analogously with the topology, that is derived from the available two-qubit gates
"two_qubit": {
"D1-D2": {

(you may consider just loading the platform, executing the create() function in the .py script, and use the qibolab.Platform interface to access these values - loading the JSON should be the heaviest operation anyhow...)

@scarrazza
Copy link
Member

@BrunoLiegiBastonLiegi please let us know when this is ready so we can merge to main and them include this in #191.

@BrunoLiegiBastonLiegi
Copy link
Author

This is ready, but it's not going to be merged in main, rather in 0.1.
I will create another PR to implement this for the current main.

@alecandido
Copy link
Member

Even #191 will be merged in 0.1. At the moment, main is still experimental, since lacking the support on the Qibocal side. So, there is no hurry.

@scarrazza scarrazza merged commit acb3b3a into 0.1 Oct 31, 2024
2 of 3 checks passed
@scarrazza scarrazza deleted the platform_readme branch October 31, 2024 04:41
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

Successfully merging this pull request may close these issues.

4 participants