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

0.2 platforms' parameters upgrader #201

Merged
merged 34 commits into from
Dec 18, 2024
Merged

0.2 platforms' parameters upgrader #201

merged 34 commits into from
Dec 18, 2024

Conversation

alecandido
Copy link
Member

@alecandido alecandido commented Nov 13, 2024

I'm not aiming to merge it anyhow, since it's just a temporary tool to face the transition.

The main motivation to have an active PR is to continuously share bug-fixes (I don't expect to be able to write something perfect at first shot).
Another good reasons is to share annotations and questions.

Of course, the target is to automatically convert just the parameters.json file, and there won't be any attempt of automated platform.py conversion.

  • check Drag.beta conversion
    • I'm not doing anything right now, but we changed the meaning of a bunch of parameters, to make them relative to the interval duration (instead of measuring them either in samples or nanoseconds), we should make sure $\beta$ was actually untouched
  • support the two_qubit section of native_gates
  • extract the relevant configs

convert.py Outdated
"duration": duration,
"amplitude": o["amplitude"],
"envelope": {},
"relative_phase": o["phase"],
Copy link
Member Author

Choose a reason for hiding this comment

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

@stavros11 in 0.1 natives only have a .phase attribute. I expect that to match the .relative_phase we're using in 0.2 (which has been directly inherited from the 0.1 Pulse object, because Pulse's serialization replaced the usage of Native).

Is it correct?

Copy link
Member

@stavros11 stavros11 Nov 14, 2024

Choose a reason for hiding this comment

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

Yes, I believe that should be fine. I actually think these phases are never used, except from the virtual-z in the CZ sequence, but it is fine to keep them as they will be deserialized anyway from the Pulse even in 0.2.

Also, not related to this comment, but I believe this is still WIP, right? I tried to use it on the qw11q and qw5q_platinum from the 0.1 branch and I am getting some errors, KeyError: 'coupler' and KeyError: 'drag' respectively. In any case, thanks for doing it, I would expect it is useful for the near term. It is not very urgent though, so no pressure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I believe that should be fine. I actually think these phases are never used, except from the virtual-z in the CZ sequence, but it is fine to keep them as they will be deserialized anyway from the Pulse even in 0.2.

Ok, thanks for the confirmation.

Also, not related to this comment, but I believe this is still WIP, right? I tried to use it on the qw11q and qw5q_platinum from the 0.1 branch and I am getting some errors, KeyError: 'coupler' and KeyError: 'drag' respectively. In any case, thanks for doing it, I would expect it is useful for the near term. It is not very urgent though, so no pressure.

Yes, there is the warning on top :P

Actually, I'm now working on further shape support (it should be ready soon - the main problem is the custom, not the drag - but the serialization is generically painful), and I'm fully missing the configs part (which will include all the frequencies, for which I will ask you again an opinion, but when I'll have something). However, I would have not expected the 'coupler' issue: could you tell me which parameters were you trying to convert?
Most likely it is just an optional attribute. But better to have an example, such that I can test :)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I'm now working on further shape support (it should be ready soon - the main problem is the custom, not the drag - but the serialization is generically painful), and I'm fully missing the configs part (which will include all the frequencies, for which I will ask you again an opinion, but when I'll have something).

Since this is going to be temporary anyway, I wouldn't worry about supporting the Custom here, since we are not using it anywhere and I don't expect us to use it, at least before a full transition to 0.2. The configs part is more relevant so I would focus there. Btw, if you need any help I can also contribute something here.

However, I would have not expected the 'coupler' issue: could you tell me which parameters were you trying to convert? Most likely it is just an optional attribute. But better to have an example, such that I can test :)

For this I think I just called python convert.py on the parameters.json of the qw11q platform from the 0.1 branch. I had to copy the platform somewhere else in order to switch back to your branch (copying convert.py would also work I guess), but you are probably already aware of this, maybe you even have a better solution. It may be even more convenient to base this on the 0.1 since if I understand correctly the goal is to convert 0.1 platforms to 0.2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is going to be temporary anyway, I wouldn't worry about supporting the Custom here, since we are not using it anywhere and I don't expect us to use it, at least before a full transition to 0.2. The configs part is more relevant so I would focus there.

I'm doing it exactly because it is part of the platform I'm trying to convert (i.e. the one in #161, that I will also use to develop Qblox).

Btw, if you need any help I can also contribute something here.

If you start thinking about how to extract the configs, I may need an informed review soon, since it is not completely clear to me that there is a unique best way of doing that.

For this I think I just called python convert.py on the parameters.json of the qw11q platform from the 0.1 branch. I had to copy the platform somewhere else in order to switch back to your branch (copying convert.py would also work I guess), but you are probably already aware of this, maybe you even have a better solution. It may be even more convenient to base this on the 0.1 since if I understand correctly the goal is to convert 0.1 platforms to 0.2.

Ah, yes. That's a good point.
I actually copied the parameters myself (I'm testing on an uncommitted old.json file). But this is just a random standalone script, intended to be kept that way. I just wanted a PR to discuss and for people to grab it, I was even unsure whether here or in the Qibolab repo itself.
But rebasing on the 0.1 seems the best option!

Copy link
Member Author

Choose a reason for hiding this comment

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

@stavros11 the problems you have spotted should now be fixed, though I have still a list of items missing. I'll write them in the OP, feel free to resolve this.

If you can start testing what is already there, and report potential issues, it would already be extremely useful.

@alecandido
Copy link
Member Author

Reminder: change formatter to ruff in the pre-commit hooks, to avoid commits like 9f4b70d...

@alecandido
Copy link
Member Author

alecandido commented Nov 14, 2024

@stavros11 @andrea-pasquale @Edoardo-Pedicillo this is now more or less ready, and you can use in a pretty straightforward way:

python convert.py qw11q/parameters.json iqm5q/parameters.json ...

which will produce new files (usually parameters-new.json).

However, it's relevant to take into account a certain amount of caveats when using automated translation:

  1. it has not been tested (for the time being), so, by using it, you'll contribute improving it :P
  2. the configs extraction is a best-effort attempt: there are many entries which should be generated which are either driver-specific, or they may require information unavailable in the former parameters

Regarding the second item, if you already have a manually translated parameters.json available, it may be worth to take part of the configs from there (part of them, since the frequencies are part of the calibration, so they have to be updated, e.g.).
We may need to extend the converter a bit, or supplement with further scripts to improve the extraction. However, the general purpose one won't be much different from the current state (I'll keep working on the Qblox driver, and possibly iterate on the driver-specific part as soon as I'll have a platform prototype to consume the calibration in #161).

@stavros11
Copy link
Member

I merged all upgrader PRs here because I want to rebase this to 0.1 in order to get the latest parameters from that branch in the platforms I am converting.

@stavros11 stavros11 marked this pull request as ready for review November 26, 2024 12:52
Copy link

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

@stavros11 stavros11 merged commit d21e5a3 into 0.1 Dec 18, 2024
3 checks passed
@stavros11 stavros11 deleted the 0.2-upgrader branch December 18, 2024 12:34
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.

3 participants