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

Refactor integration weights protocol #1099

Merged
merged 24 commits into from
Mar 11, 2025
Merged

Refactor integration weights protocol #1099

merged 24 commits into from
Mar 11, 2025

Conversation

andrea-pasquale
Copy link
Contributor

@andrea-pasquale andrea-pasquale commented Feb 24, 2025

The previous implementation of the protocol to optimize the integration weights was based on LaboneQ and I don't think that we ever used it with QM. I've refactored the code to have a more instrument agnostic version which is assuming that with AcquisitionType.RAW the samples are returned without demodulation.
In the protocol I am now doing the demodulation on software (with the addition of a low pass filter to remove fast-rotating terms $\propto \cos(2\omega_{IF}t)$.

It seems that the readout fidelity improves http://login.qrccluster.com:9000/Clqog-92SlKblXqbMVo-Vw==/, however it might be good if someone else takes a look at the code.

Requires a few changes in the QM driver qiboteam/qibolab#1155

@andrea-pasquale
Copy link
Contributor Author

I believe this should be ready to review. Tests are not passing because with the new post-processing we need to compute the intermediate frequency, however dummy doesn't have LOs. This problem has been fixed in #1101 which is pointing to this PR.

@andrea-pasquale andrea-pasquale marked this pull request as ready for review February 25, 2025 13:54
Copy link
Contributor

@Edoardo-Pedicillo Edoardo-Pedicillo left a comment

Choose a reason for hiding this comment

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

It looks good to me, some documentation of the fit function is missing.

Copy link

codecov bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.79%. Comparing base (0068da2) to head (98def3a).
Report is 25 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1099      +/-   ##
==========================================
- Coverage   97.95%   97.79%   -0.16%     
==========================================
  Files          99       99              
  Lines        8072     8087      +15     
==========================================
+ Hits         7907     7909       +2     
- Misses        165      178      +13     
Flag Coverage Δ
unittests 97.79% <100.00%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/qibocal/__init__.py 100.00% <100.00%> (ø)
src/qibocal/auto/execute.py 95.38% <100.00%> (-2.31%) ⬇️
src/qibocal/auto/runcard.py 97.50% <100.00%> (ø)
src/qibocal/calibration/platform.py 100.00% <100.00%> (ø)
...gnal_experiments/calibrate_state_discrimination.py 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

drive12 = f"{q}/drive12"
qubits[q] = qubit = Qubit.default(q, drive_qudits={(1, 2): drive12})
channels |= {
qubit.probe: IqChannel(device="pump_name", mixer=None, lo="01/probe_lo"),
Copy link
Member

Choose a reason for hiding this comment

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

Not very important but why is the device="pump_name"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably I it should have been an f-string or I should have just passed pump_name.
Thanks for catching this.

Copy link
Member

@stavros11 stavros11 Mar 11, 2025

Choose a reason for hiding this comment

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

I think the pump_name is for the TWPA pump local oscillator, which we are already passing to the AcquisitionChannel. For the probe channel it should probably be a different device (like qblox, QM, etc.), but for the mock platform it will probably work even without device, since you are not passing any device for the drive and flux either. That's also why the device does not really matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, at this point we can just remove it if it is not mandatory.

andrea-pasquale and others added 3 commits March 11, 2025 09:09
Co-authored-by: Stavros Efthymiou <35475381+stavros11@users.noreply.github.com>
@andrea-pasquale andrea-pasquale added this pull request to the merge queue Mar 11, 2025
Merged via the queue into main with commit 67db425 Mar 11, 2025
39 checks passed
@andrea-pasquale andrea-pasquale deleted the kernels branch March 11, 2025 14:10
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