Simplify single shot protocols#1278
Conversation
eae0fd8 to
9148d2d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1278 +/- ##
==========================================
- Coverage 96.86% 96.75% -0.11%
==========================================
Files 133 131 -2
Lines 10457 10139 -318
==========================================
- Hits 10129 9810 -319
- Misses 328 329 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I believe that this should be ready to review now. In the last report you can see how by lowering the readout amplitude we get better QND reducing the excitations to state 2. |
for more information, see https://pre-commit.ci
027ae14 to
c107378
Compare
Everything is possible :)
I agree, it would make sense that if you say you are measuring the same thing, they should be in the same position. @lballerio will certainly take a look into this (as soon as he's done with some of the other open PRs 🙄) |
Add average states markers in ssro plots and change the scaling so that the clusters are more visible.
for more information, see https://pre-commit.ci
|
The plots in the SSRO stretch ugly with the report window; in #1334 I added an anchor to the x axis that helps the plot be "prettier" and added the average state markers. |
|
Hi @jevillegasdatTII, did you open an issue? |
modified: src/qibocal/protocols/classification/classification.py
jevillegasd
left a comment
There was a problem hiding this comment.
im ok with the changes as they are and it can be worked around the smaller fixes commented (sweeps being realitve) afterwards.
|
@lballerio is about to double-check this with QPU 118, as soon as he gets suitable frequencies for the qubits. Whenever he confirms as well, we can merge |
lballerio
left a comment
There was a problem hiding this comment.
Here is my review about this PR...some comments are docs/ minor fixes, but I think some comments, especially about 1->2 transition experiments are more relevant.
However I briefly tested the experiments and they seem to work, but I would not merge now the PR.
| html_list = [] | ||
| for figure in figures: | ||
| figure.write_html(buffer, include_plotlyjs=False, full_html=False) | ||
| figure.write_html(buffer, include_plotlyjs="cdn", full_html=False) |
There was a problem hiding this comment.
I want to link write_html help function and focus on the discussion about include_plotlyjs parameter:
Long story short, this configuration of include_plotlyjs requires an active internet connection (except if cached).
Is it what we want?
There was a problem hiding this comment.
Yes, that's reasonable. Otherwise, each report would be just 3 MB larger, only because of repeated content. Most of the time, you also need to be connected to the internet to retrieve these reports.
The only other interesting option would be directory, and self-host the JavaScript package.
This is fine, but requires more effort on our side. Also because of potential upgrades and compatibility concerns.
Until this becomes explicitly a problem, I would keep cdn.
| ROC_LENGHT = 800 | ||
| ROC_WIDTH = 800 | ||
| DEFAULT_CLASSIFIER = "qubit_fit" |
There was a problem hiding this comment.
ROC_LENGHT, ROC_WIDTH and DEFAULT_CLASSIFIER are not used in this script nor in other files.
| ROC_LENGHT = 800 | |
| ROC_WIDTH = 800 | |
| DEFAULT_CLASSIFIER = "qubit_fit" |
There was a problem hiding this comment.
They were, before this PR.
Since now they are not any longer used, it makes perfect sense to remove them. Good catch :)
| platform.channels[platform.qubits[q].drive].lo: { | ||
| "frequency": platform.config(platform.qubits[q].drive).frequency | ||
| + params.lo_offset * GHZ_TO_HZ | ||
| }, |
There was a problem hiding this comment.
same as previous comment, here we are moving LO frequencies, don't we have to recalibrate mixers then?
There was a problem hiding this comment.
Yes, we should: much better to drop this shift, and keep it manual (for the time being).
If strongly needed, we could automate this process later on, when we will have stabilized the mixer calibration API.
There was a problem hiding this comment.
I think here the file name and the protocol name is misleading.
I think ef.py is quite meaningless, and also I realized that this is a signal experiment, not a probability, but we export it as rabi_amplitude_ef, which is inconsistent with all other protocols where we can measure both in probability and signel+phase.
Additionally, I was wondering if it is worth to put rabi and rabi_ef all together in a single protocol and use a keyword to trigger either 0->1 or 1->2 transitions since the experiments are the same and rabi_ef code often recall rabi classes and fuctions.
Also might be worth to implement all Rabi experiments even for 1->2 transition (?)
The bottleneck might be that is quite boring and refactoring the code is time consuming.
There was a problem hiding this comment.
I think
ef.pyis quite meaningless, and also I realized that this is asignalexperiment, not aprobability, but we export it asrabi_amplitude_ef, which is inconsistent with all other protocols where we can measure both in probability and signel+phase.
Agreed. We could call the file amplitude_signal_ef.py and the protocol rabi_amplitude_signal_ef.
Additionally, I was wondering if it is worth to put
rabiandrabi_efall together in a single protocol and use a keyword to trigger either 0->1 or 1->2 transitions since the experiments are the same andrabi_efcode often recallrabiclasses and fuctions.
Also might be worth to implement all Rabi experiments even for1->2transition (?)
The bottleneck might be that is quite boring and refactoring the code is time consuming.
It could be worth, to deduplicate code. But let's not do it in this PR. You could open an issue about it.
| signal=result.T[0], | ||
| phase=result.T[1], |
There was a problem hiding this comment.
are we sure results are signal phase and magnitude and not I-Q components?
There was a problem hiding this comment.
Actually, since they are coming directly from Qibolab, they are exactly IQ pairs.
This is a plain bug
| qubit: float(platform.calibration.single_qubits[qubit].qubit.frequency_01) | ||
| for qubit in targets |
There was a problem hiding this comment.
shouldn't this be the drive frequency instead?
this is what I was thinking about:
| qubit: float(platform.calibration.single_qubits[qubit].qubit.frequency_01) | |
| for qubit in targets | |
| qubit: float(platform.config(platform.qubits[qubit].drive).frequency) | |
| for qubit in targets |
| # # mask values where fidelity is below 80% | ||
| # averaged_qnd[grids["fidelity"] < 0.8] = np.nan | ||
| # # exclude values where QND is larger than 1 | ||
| # averaged_qnd[averaged_qnd > 1] = np.nan |
There was a problem hiding this comment.
indeed, in my last calibration with qpus 118 and 156 I had problems with the old implementation of this protocol since all fidelities where <80%, hence all the data were np.nan and the experiment failed. I think this mask is useless, or at least we can decrease the threshold to 50%.
| # # mask values where fidelity is below 80% | |
| # averaged_qnd[grids["fidelity"] < 0.8] = np.nan | |
| # # exclude values where QND is larger than 1 | |
| # averaged_qnd[averaged_qnd > 1] = np.nan |
There was a problem hiding this comment.
No, I would not put it to 50%. I agree it is just useless.
If we want to consider taking a different point, we can engineer a suitable cost function (not necessarily linear).
Same thing for the plots: if we want to avoid losing details in the high-fidelity patch, we could even change the colorscale with a non-linear rescaling. Though, it is possibly not even needed.
alecandido
left a comment
There was a problem hiding this comment.
I mostly read @lballerio's comments, and the related code. However, I also had a quick look to some of the other changes.
All in all, conditional to the result to be functioning, we do not need a detailed review of all the code changes. Which are massive, mostly because of a huge refactoring (but you could assume the old code to be messy and unreliable - no need to spend ages navigating the diff).
However, as @lballerio already pointed out, there is at least one outright bug (if I'm not missing anything myself). And I would implement the small improvements he suggested, postponing the larger changes (possibly opening issues, if relevant).
|
Btw, once the review is done, the PR needs to be rebased. Though it should be straightforward, since GitHub is not reporting any conflict |








Closes #1241
In this PR I'm planning to simplify the code related to the single experiments.
Some modifications include: