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

Couplers2 #623

Merged
merged 23 commits into from
Nov 3, 2023
Merged

Couplers2 #623

merged 23 commits into from
Nov 3, 2023

Conversation

Jacfomg
Copy link
Contributor

@Jacfomg Jacfomg commented Oct 17, 2023

Changes to adress the coupler and sweep their bias. Works with qibocal qiboteam/qibocal#565 and qibolab_platforms []

Checklist:

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

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

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

Files Coverage Δ
src/qibolab/couplers.py 90.90% <100.00%> (+1.43%) ⬆️
src/qibolab/native.py 99.45% <100.00%> (+4.91%) ⬆️
src/qibolab/pulses.py 92.26% <100.00%> (+0.25%) ⬆️
src/qibolab/serialize.py 100.00% <100.00%> (ø)
src/qibolab/platform.py 78.31% <59.09%> (-2.17%) ⬇️
src/qibolab/instruments/zhinst.py 77.81% <71.73%> (-0.75%) ⬇️

📢 Thoughts on this report? Let us know!.

@Jacfomg Jacfomg marked this pull request as ready for review October 18, 2023 07:48
@Jacfomg
Copy link
Contributor Author

Jacfomg commented Oct 23, 2023

Should we also add coupler pulses into the CZ so they get tested as well ?

@stavros11
Copy link
Member

Should we also add coupler pulses into the CZ so they get tested as well ?

Yes, I think that would be good.

@Jacfomg
Copy link
Contributor Author

Jacfomg commented Oct 25, 2023

Btw, @stavros11 any clue on the qm tests failing before I take a look ?

Copy link
Contributor

@GabrielePalazzo GabrielePalazzo left a comment

Choose a reason for hiding this comment

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

Apart from the only remark I have, it looks ok to me.

src/qibolab/instruments/zhinst.py Outdated Show resolved Hide resolved
@Jacfomg
Copy link
Contributor Author

Jacfomg commented Nov 1, 2023

Sorry to add this after your review @GabrielePalazzo I just found some timing issues when inspecting the pulse sequences being played on the device on the coupler routines.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. I did not review the driver update in detail as I am hoping to do a general pass over drivers soon, but otherwise looks good.

src/qibolab/dummy.yml Show resolved Hide resolved
src/qibolab/platform.py Outdated Show resolved Hide resolved
src/qibolab/instruments/zhinst.py Outdated Show resolved Hide resolved
src/qibolab/platform.py Outdated Show resolved Hide resolved
src/qibolab/pulses.py Outdated Show resolved Hide resolved
@Jacfomg
Copy link
Contributor Author

Jacfomg commented Nov 2, 2023

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @Jacfomg. I believe there is an issue with dumping platforms that contain pulses to couplers, for example if you try the following

from qibolab import create_platform
from qibolab.serialize import dump_runcard
from pathlib import Path

platform = create_platform("dummy")
dump_runcard(platform, Path(__file__).parent / "dummy_dump.yml")

it should dump the dummy runcard in a new yaml named dummy_dump.yml. Ideally this should look the same with the original dummy.yml, but if you look at it, the whole coupler section is missing from the native gates and the CZ/iSWAPs are also missing the coupler pulses.

src/qibolab/native.py Outdated Show resolved Hide resolved
src/qibolab/platform.py Outdated Show resolved Hide resolved
src/qibolab/platform.py Outdated Show resolved Hide resolved
Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. Looks good to me now.

src/qibolab/serialize.py Outdated Show resolved Hide resolved
@Jacfomg Jacfomg merged commit d852e82 into main Nov 3, 2023
21 checks passed
@stavros11 stavros11 deleted the couplers2 branch November 3, 2023 09:40
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