-
Notifications
You must be signed in to change notification settings - Fork 7
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
Small fixes for TWPA calibration routines #899
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #899 +/- ##
==========================================
- Coverage 97.48% 97.44% -0.05%
==========================================
Files 116 116
Lines 8882 8881 -1
==========================================
- Hits 8659 8654 -5
- Misses 223 227 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @stavros11, I agree with all the fixes.
Regarding the relaxation I already had a feeling that it was not used correctly in all protocols (#829).
src/qibocal/protocols/readout_optimization/twpa_calibration/frequency_power.py
Outdated
Show resolved
Hide resolved
) | ||
|
||
for power in power_range: | ||
for qubit in targets: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for qubit in targets: |
At this point, I suggest removing this line and leaving the indentation as in main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, both comments are right. I reverted the indentation and removed this in 79cd1cb.
The routine should now work, but we should keep in mind that it is suboptimal and maybe open an issue for this. The acquisition is repeated in multiplexed fashion on all qubits multiple times and in each case we drop the results for all qubits except of one. We should only need one acquisition per TWPA pump, not per qubit. For 5-qubits that share a readout line, which is the case for most of our current chips, we should only need one acquisition, not five.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have already opened #564 , for sure what we are doing is wrong, but we could collect all the results and find a better way to use them
Some issues I realized while trying to use these routines:
frequency_power
was giving someKeyError
s withqubit
s missing from theinitial_twpa_freq
andinitial_twpa_power
dictionaries. I believe these are because the execution part of the routine is nested in the loop that initializates these dictionaries, while I think it should be a separate loop.relaxation_time
is ignored for both SNR routines as it is not passed in theresonator_spectroscopy
call. This ends up using the defaultrelaxation_time
from the platform runcard, which is usually much higher than what we need for spectroscopy, thus making execution slower.Checklist:
master
main
main