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

Zurich measurement sequences start at time set in Pulses #685

Merged
merged 31 commits into from
Dec 13, 2023

Conversation

GabrielePalazzo
Copy link
Contributor

Checklist:

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

Copy link

codecov bot commented Dec 4, 2023

Codecov Report

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

Comparison is base (7cbaaa6) 62.15% compared to head (a143ceb) 62.09%.

Files Patch % Lines
src/qibolab/instruments/zhinst.py 94.31% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #685      +/-   ##
==========================================
- Coverage   62.15%   62.09%   -0.07%     
==========================================
  Files          49       49              
  Lines        6834     6857      +23     
==========================================
+ Hits         4248     4258      +10     
- Misses       2586     2599      +13     
Flag Coverage Δ
unittests 62.09% <94.31%> (-0.07%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jacfomg
Copy link
Contributor

Jacfomg commented Dec 4, 2023

Also, this crashes when multiple measurements are applied on the same sequence. Let's say, readout_characterization routine for example.

@Jacfomg Jacfomg marked this pull request as draft December 4, 2023 10:45
@GabrielePalazzo GabrielePalazzo changed the base branch from main to Zurich_freq_sweepers December 6, 2023 11:24
@GabrielePalazzo GabrielePalazzo changed the base branch from Zurich_freq_sweepers to main December 6, 2023 11:24
@GabrielePalazzo GabrielePalazzo marked this pull request as ready for review December 11, 2023 06:59
Copy link
Contributor

@Jacfomg Jacfomg left a comment

Choose a reason for hiding this comment

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

Thanks @GabrielePalazzo the code looks good, and works as intented in the existing routines I tried. I just have some small format comments. I would test with an unrolling routine soon before approving.

src/qibolab/instruments/zhinst.py Outdated Show resolved Hide resolved
pulse_sequences.append([])
measurement_index = 0
for pulse in pulses:
if measurement_index < len(measurements):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if measurement_index < len(measurements):
if measurement_index < len(measurements) and isinstance(quantum_element, Qubit):

How about something like this as we will never read couplers ?

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

Jacfomg commented Dec 11, 2023

I was trying to see if I could reproduce the same results with and without the unrolling using qiboteam/qibocal#629. As the qubits were not calibrated I could see anything so I did qiboteam/qibocal#665. The results seem the same so that means no phase reset on the measurament or whatever is needed.

@Jacfomg
Copy link
Contributor

Jacfomg commented Dec 11, 2023

@GabrielePalazzo there is an issue with the T1 sweepers

Copy link
Contributor

@Jacfomg Jacfomg left a comment

Choose a reason for hiding this comment

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

Everything seems working as intended. The only thing left would be to update the number of sequences in a smart way they can take for the unrolling in a later benchmark. Thanks @GabrielePalazzo.

@Jacfomg
Copy link
Contributor

Jacfomg commented Dec 12, 2023

I merged main bcause #629 and found a typo on my code.

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 @GabrielePalazzo @Jacfomg. I tested this with allXY and unrolling and the results seem compatible with the no unrolling case, so I guess the measurement order has been fixed.

@@ -55,8 +55,8 @@
SWEEPER_BIAS = {"bias"}
SWEEPER_START = {"start"}

MAX_MEASUREMENTS = 32
"""Maximum number of readout pulses in a single sequence."""
MAX_SEQUENCES = 150
Copy link
Member

Choose a reason for hiding this comment

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

I have not done extensive benchmarking about this, but as a general note it is better to underestimate than push it to its limit, because execution will fail completely if we exceed the memory. While if we underestimate, it may take a bit longer because it will split more batches but it will still work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree, I found that for RB like circuits it would be like 20 while for simple calibration experiments it was around 150. So it is a huge change, I would suggest to count the number of pulses in the sequence as part of the batching maybe.

@GabrielePalazzo GabrielePalazzo merged commit 8c0a7ce into main Dec 13, 2023
20 checks passed
@GabrielePalazzo GabrielePalazzo deleted the zh_measurement_sequences branch December 13, 2023 05:00
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