collection of small fixes, features, and random pieces of code #993
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 0.1 #993 +/- ##
==========================================
- Coverage 70.07% 69.82% -0.26%
==========================================
Files 64 64
Lines 6771 6859 +88
==========================================
+ Hits 4745 4789 +44
- Misses 2026 2070 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@hay-k I am still reviewing the PR. Sorry, but after the last warm up I had to re calibrate the chip using the old driver in order to compare the results with your version of the qblox driver. I am finishing the 2q gates so, I will report what I see at the end of the week. |
|
Hi @hay-k I am trying to reproduce the same results that we are getting with the old version for the driver and your PR. I am trying to reproduce the same results for the 2q gates running the iSWAP and CZ chevron. This routines are working perfectly with the old version of the driver: Classify q1 q2
With the latest version of the runcard (same used in the old driver): And it is throwing the error: Traceback (most recent call last): I have tried setting the qf and coupler pulse to 200ns, that is the same number of samples of the custom shape used during this routine, but same error is raised. It would be nice to check this routine, because it involves many of the fixes that you have introduced. Any idea what is the problem? |
|
@DavidSarlle is this experiment sweeping the duration of the custom pulse? |
Yes, it is. The flux pulse is using custom shape, needed to get good chevrons. I know that the error is raised because the number of samples is changing during the sweeping. But this is something necessary and was fixed long time ago in Alvaro's branch because qblox does not support predistorted pulses yet. By the way, I will write today a detailed report with the differences observed between branches. |
|
How is this type of sweep supposed to be interpreted? How does one sweep the duration of a pulse that is specified sample by sample and has fixed duration? I have checked what Alvaro has done in his branch. It pretty much boils down to this line. The variable |
|
All results has been made with iqm5q qubit q1/q4 trying to minimizze the tiome between execution of the same routine. (Old driver) qibocal -> alvaro/latest_20231215 New driver: qibocal -> main classifyalvaro: http://login.qrccluster.com:9000/W4Bb2lEmRmSm1_BPuZLAaA== COMMENTS: All results matching qubit spec fluxalvaro: http://login.qrccluster.com:9000/EYdEAkTgRk2PWPA72WjohQ== COMMENTS: Phase results not matching qubit specalvaro: http://login.qrccluster.com:9000/eMJF7vRlQwKpH5heYWckHw== COMMENTS: Phase results not matching rabi amplitudealvaro: http://login.qrccluster.com:9000/_kpHtQ08ReyxxRc_VpjAKg== COMMENTS: Phase results not matching rabi lengthalvaro: http://login.qrccluster.com:9000/hHC9WWizS8a4TxmUDlwmzA== COMMENTS: Probably RO pulse not well allocated during sweep and pulse sequence duration changed, as we pointed out long time ago. ramseyalvaro: http://login.qrccluster.com:9000/txpGzRxwRzG2dDG1H9gxRA== COMMENTS: Signal results not matching. Probably RO pulse not well allocated during sweep and pulse sequence duration changed, as we pointed out long time ago. T1alvaro: http://login.qrccluster.com:9000/A3ebtszyTmKpRrC9-BEu-A== COMMENTS: Signal results not matching. Probably RO pulse not well allocated during sweep and pulse sequence duration changed, as we pointed out long time ago. T2alvaro: http://login.qrccluster.com:9000/I8j2GDt6SMWqGwNplVkz2Q== COMMENTS: Signal results not matching. Probably RO pulse not well allocated during sweep and pulse sequence duration changed, as we pointed out long time ago. OTHER PROBLEMS:
|
@aorgazf can give you more details. But that was the idea that we had to fix the problem. It works. as soon as we implemented, we were able to characterize the couplers and 2q gates. Before, the flux pulses were far away of being perfect and we needed to pre-distort them. As soon as we did it, we started to see the chevrons. @hay-k if you have any othe better solutions, feel free to introduce it and test it. |
It is not: you should either compute the samples from a function, or interpolate the given ones. SciPy has easy-to-use interpolators that you could use for that |
|
Hi, |
|
Hi @aorgazf, this is more or less becoming a performance optimization issue. Indeed, I see that what you say could be used (though I won't enter the details of the specific protocols, as I'm not experienced enough with that), but it's also not the general expected behavior. We had a similar problem with QM, that has the option to interpolate the waveform in real time. The solution we're proposing with @stavros11 in 0.2 is to split the sweeper in two:
So, if this kind of sweeper you're proposing is relevant (as said, for the sake of the current discussion I consider it relevant), it could be implemented in the same way, as a duration cut sweeper. I'm mostly talking about the long-run development of Qibolab (i.e. 0.2+), since I'd like to support more and more features, but improving the standardization and compatibility at the same time. |
|
another example of results mismatching: Landscapealvaro: http://login.qrccluster.com:9000/x0Wj_DoPTWCL_8ob5hAehw== COMMENTS: Signal and/or phase results not matching. Probably RO pulse not well allocated during sweep and pulse sequence duration changed, as we pointed out long time ago. We need to be able to run the landscape to fine tuning the CZs |
I believe this is not an issue in the driver, it is because of some difference in data processing between the two versions of software.
This was thoroughly discussed in #919 and in a few meetings with @aorgazf. The short summary is that this cannot be consistently fixed in qibolab until qibolab 0.2 is released. Why? The current qibolab API is limited and does not allow user to specify whether they want to move RO when the start or duration of other pulses is changed, or they want to keep RO in the same place always. Drivers cannot be implemented so that they handle both cases correctly - they always do the same thing regardless of what user requests (e.g. ZI driver always pushes the RO to the end, and the qblox driver always plays RO in a fixed place). It was concluded that for now it is a good consensus to implement all new features (e.g. couplers, mixer calibration, fix deserialization of Custom pulse, etc.), but don't fix the RO pulse start time sweep problems. |
|
Anyways, The original idea behind this PR was to implement things nicely, in a way that makes sense in general and not in the context of specific experiments and platforms only. Thus the PR could be merged and a new version of qibolab released. Given the fact that so far not all discussions/conclusions have been kept by all parties, and my opinion that there is no point in further discussions attempting to do things the proper way, I propose the following:
This means:
|
|
@hay-k at this stage please go ahead with this proposal. Qibolab 0.1 should contain the required patches until 0.2 takes over. |
hay-k
changed the title
|
In line with the proposal above, I have now pushed two commits to this branch:
2. added the functionality to cut custom pulses to their first samples when their duration is swept, as discussed above.
Only remaining thing is the case where the RO needs to be pushed when the duration of a drive pulse is swept (e.g. Rabi length experiment). Turns out I didn't have a ready workaround for this one, so I will try to figure out something a bit later, and push a new commit. |
|
Thanks @hay-k . Please try to reproduce the reports uploaded to be sure that the fixes introduced are working. Also remember the problems with displaying/manipulating the phase data in many routines. Try to reproduce yourself the landscape report and other reports like qubit spec where this issue can be observed. |
|
Now I have a workaround for the case of duration sweeper as well, and Rabi length experiment works fine: 
@DavidSarlle I didn't try the chevron experiment, can you test that one?
Is this really a problem? |
Nevermind. I found where this difference is coming from. It should not cause any problems because this phase calculations are done only for the plots in the signal experiments, but anyways I changed it so that it looks like as it did in the past. |
|
Thanks @hay-k . I have tested right now the landscape and it is still not working. Thanks for fixing the phase problem, but maybe there is still something else that is not working as expected as you can see in the landscape. This is the reason why I said that the phase manipulation was a problem. http://login.qrccluster.com:9000/FzsNOANnT3W6LeXULiOxew== Also all the drivers supported by qibo are displaying the phase in that way. |
|
@hay-k I tested the chevrons, and now is not raising any bug, but the results are not good: alvaro: http://login.qrccluster.com:9000/jyjXrMv6Q0qCFz8WbYKlKg== Please note that the chevron routine is using all the fixes introduced. |
|
@DavidSarlle This is because your platform definition for the new version of software is incomplete - the platform does not know that it has couplers, which results into the coupler pulse not being applied. To fix this you need to do the following change: return Platform(
- str(FOLDER), qubits, pairs, instruments, settings, resonator_type="2D"
+ str(FOLDER), qubits, pairs, instruments, settings, resonator_type="2D", couplers=couplers
)With this change I can see nice chevron similar to what you get from the old versions: |
…e RO according to the sweep
hay-k
force-pushed
the
qblox-ad-hoc-features
branch
2 times, most recently
from
add0158
to
21e795b
Compare
hay-k
force-pushed
the
qblox-ad-hoc-features
branch
from
21e795b
to
035b325
Compare
|
@hay-k I'm not sure if you already encountered, but do not worry about |
There was a problem hiding this comment.
This PR is mostly affecting just Qblox, and that was confirmed to work by @DavidSarlle.
I checked the changes, and those beyond the scope of the driver itself are very limited. I'm reporting a sign in the phases below (to be discussed there), while platform/platform.py is affected only to tune flux pulses, in a backward compatible way.
pulses.py is more heavily changed, but mostly just for the (de)serialization and waveforms (which are only used by Qblox itself), as well as a method of the PulseSequence, which I also confirmed is only used by Qblox.
It should be ready to be merged, but I will wait just one more day for some final tests (just to be completely sure).
Thanks @hay-k for these fixes!
(btw, you didn't mark as ready for review, but I will leave you to merge in any case)
| @@ -38,7 +38,7 @@ def magnitude(self): | |||
| @cached_property | |||
| def phase(self): | |||
| """Signal phase in radians.""" | |||
| return np.unwrap(np.arctan2(self.voltage_i, self.voltage_q)) | |||
| return np.unwrap(np.arctan2(self.voltage_q, self.voltage_i)) | |||
There was a problem hiding this comment.
So, I'm not sure why you are reversing the two arguments, but this seems to be different from what we used until now.
Even in 0.2, we were computing the phase as
https://github.com/qiboteam/qibocal/blob/d35ab6b2d707031520927a9325b62b9b95a60c62/src/qibocal/result.py#L78
i.e.
while you're using:
This is mostly a sign, since
(i.e. a sign and an offset, which should be taken away from the unwrapping and cable delay removal).
It's not extremely relevant, and I don't have any special reason to push for one sign or the other. I'm just trying to track the changes.
There was a problem hiding this comment.
@alecandido The reason is the numpy's definition of the arctan2 function. The phase (or angle as in np.angle) of a complex number a+jb is np.arctan2(b, a) and not np.arctan2(a, b).
|
@hay-k can we merge this today? |
This PR brings the following:
offset_iandoffset_qmixer calibration parameters for qblox RF modules.Custompulse shape:This is intended to cover the discussions in #919 and #921