RX90 calibration implementation #1044

ElStabilini · 2024-11-20T13:07:17Z

Checklist:

Reviewers confirm new code works as expected.
Tests are passing.
Coverage does not decrease.
Documentation is updated.
Compatibility with Qibo modules (Please edit this section if the current pull request is not compatible with the following branches).
- Qibo: master
- Qibolab: main
- Qibolab_platforms_qrc: main

codecov · 2024-11-20T13:09:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.05%. Comparing base (4a1251a) to head (af82fec).

Additional details and impacted files

@@            Coverage Diff             @@
##              0.2    #1044      +/-   ##
==========================================
+ Coverage   97.04%   97.05%   +0.01%     
==========================================
  Files          98       98              
  Lines        7893     7958      +65     
==========================================
+ Hits         7660     7724      +64     
- Misses        233      234       +1

Flag	Coverage Δ
unittests	`97.05% <100.00%> (+0.01%)`	⬆️

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

Files with missing lines	Coverage Δ
src/qibocal/protocols/flipping.py	`98.64% <100.00%> (+0.18%)`	⬆️
src/qibocal/protocols/rabi/amplitude.py	`96.87% <100.00%> (+0.10%)`	⬆️
src/qibocal/protocols/rabi/amplitude_frequency.py	`97.95% <100.00%> (+0.02%)`	⬆️
...bocal/protocols/rabi/amplitude_frequency_signal.py	`98.46% <100.00%> (+0.08%)`	⬆️
src/qibocal/protocols/rabi/amplitude_signal.py	`97.59% <100.00%> (+0.18%)`	⬆️
src/qibocal/protocols/rabi/ef.py	`100.00% <100.00%> (ø)`
src/qibocal/protocols/rabi/length.py	`96.20% <100.00%> (+0.09%)`	⬆️
src/qibocal/protocols/rabi/length_frequency.py	`97.05% <100.00%> (+0.02%)`	⬆️
.../qibocal/protocols/rabi/length_frequency_signal.py	`97.79% <100.00%> (+0.11%)`	⬆️
src/qibocal/protocols/rabi/length_signal.py	`96.59% <100.00%> (+0.24%)`	⬆️
... and 2 more

... and 2 files with indirect coverage changes

ElStabilini · 2024-11-20T13:38:35Z

I attach here some reports where I tested the code:

rabi_amplitude routine: RX($pi$), RX($\pi/2$)
rabi_amplitude_frequency_routine: RX($\pi$), RX($\pi/2$)
rabi_length routine: RX($\pi$), RX($\pi/2$)
All $\pi/2$ routines were executed using the main platform in and the code from ec4b7ad, while $\pi$ routines were run using qibocal branch 0.2

andrea-pasquale · 2024-11-20T15:13:25Z

Thanks @ElStabilini, the results seem to make sense since for both amplitude and duration there is a factor 2. I think that rather than changing the protocol completly we could consider adding a flag to specify whether we want to calibrate the RX or RX90 pulse. We should also check the corresponding update function in the protocols to be consistent.
Eventually we can do the same for both the flipping and the drag calibration. For now let's finish first rabi.

src/qibocal/update.py

src/qibocal/protocols/rabi/length_signal.py

src/qibocal/protocols/rabi/length_frequency_signal.py

ElStabilini · 2024-11-21T07:47:31Z

Thanks @ElStabilini, the results seem to make sense since for both amplitude and duration there is a factor 2. I think that rather than changing the protocol completly we could consider adding a flag to specify whether we want to calibrate the RX or RX90 pulse. We should also check the corresponding update function in the protocols to be consistent. Eventually we can do the same for both the flipping and the drag calibration. For now let's finish first rabi.

@andrea-pasquale I see this only now, I should have done this by adding the pihalf_pulse (soon rx90) option, correct?
Regarding routines, should I change them even though I added a new RX90 native gate and left untouched the RX?

for more information, see https://pre-commit.ci

Merge branch 'pi_half' of github.com:qiboteam/qibocal into pi_half

We changed the default runcard option to False (by default will calibrate pi pulse)

andrea-pasquale

Thanks for the updates @ElStabilini.
At this point I would rename everywhere pihalf_pulse to rx90.
Everything else looks good to me.

doc/source/protocols/rabi/rabi.rst

andrea-pasquale · 2024-11-21T12:30:11Z

src/qibocal/protocols/rabi/ef.py

+    update.drive_12_amplitude(
+        results.amplitude[target], results.pihalf_pulse, platform, target


Here we should force the user to calibrate just with RX, given that we have no RX1290 pulse.

I should have fixed this too, in this commit 349e312.

If the user sets rx90: True it will raise an error

andrea-pasquale · 2024-11-21T12:31:30Z

doc/source/protocols/rabi/rabi.rst

+        relaxation_time: 50000
+	RX90: False


This line needs to be fixed.

I should have fixed it and also added the figure

Merge branch 'pi_half' of github.com:qiboteam/qibocal into pi_half

andrea-pasquale · 2024-11-21T14:33:35Z

Regarding routines, should I change them even though I added a new RX90 native gate and left untouched the RX?

Just to avoid too many things at the same time I suggest to focus first on rabi experiments. In another PR we can propagate these changes to the other protocols.

for more information, see https://pre-commit.ci

alecandido · 2024-11-27T20:27:37Z

Someone noticed a certain streak of pretty close commit messages (once in a while, I was not the first one - I won't take this merit :P).

Commit messages are extremely useful for debugging, to navigate the commits in case something goes wrong. So, ideally, they should describe in the most succinct and expressive way the commits' changes.
Think about searching for some change by yourself, which you don't remember exactly when and where it happened, or if you need to reprocess commits (e.g. during a rebase).

Of course, sometimes you need to push to debug the workflow. But this is better done with a local reproduction as much as possible. And, if really needed, possibly even squashing some failed attempts afterward, to leave a better history for later :)

for more information, see https://pre-commit.ci

Merge branch 'pi_half' of github.com:qiboteam/qibocal into pi_half

…alibrated

Merge branch 'pi_half' of github.com:qiboteam/qibocal into pi_half

alecandido · 2024-11-29T13:58:46Z

src/qibocal/protocols/rabi/amplitude_frequency.py

+        if data.rx90:
+            pulse_name = "Pi-half pulse"
+        else:
+            pulse_name = "Pi pulse"


In these cases, it is especially convenient to use the ternary if operator (exact same of the ... ? ... : ... construct in C)

Suggested change

if data.rx90:

pulse_name = "Pi-half pulse"

else:

pulse_name = "Pi pulse"

pulse_name = "Pi-half pulse" if data.rx90 else "Pi pulse"

alecandido · 2024-11-30T07:08:32Z

@ElStabilini just to provide you some context about #1044 (comment), the clear advantage of the proposed syntax is neither saving characters nor lines.

The most prominent difference between the equivalent expressions is that one is imperative, the other functional.
In the first one, you or conditioning the entire assignment operation, as the process of defining a variable name and its associated value. In principle, you need to analyze both branches of the conditional to ensure that the variable (pulse_name in this case) is even defined.
Instead, when using the ternary operation you are not conditioning the assignment as an operation, which becomes just an alias for a given computation. The computation is an expression, that always has a value. But it's the value itself the one being conditioned.

The functional approach makes the computation flow clearer, since it is somehow more rigid (you're not even thinking about operations, and possibly memory, but just values manipulation), and makes the analysis simpler and more effective. In languages which are actually functional, this may empower the compiler to act more aggressively. In Python, it makes the linters more effective[*], leading to fewer errors and more helpful messages :)

[*]: since it optimizes the simplicity of the code, it makes it also more human-readable in general

ElStabilini · 2024-11-30T07:10:37Z

@ElStabilini just to provide you some context about #1044 (comment), the clear advantage of the proposed syntax is neither saving characters nor lines.

The most prominent difference between the equivalent expressions is that one is imperative, the other functional. In the first one, you or conditioning the entire assignment operation, as the process of defining a variable name and its associated value. In principle, you need to analyze both branches of the conditional to ensure that the variable (pulse_name in this case) is even defined. Instead, when using the ternary operation you are not conditioning the assignment as an operation, which becomes just an alias for a given computation. The computation is an expression, that always has a value. But it's the value itself the one being conditioned.

The functional approach makes the computation flow clearer, since it is somehow more rigid (you're not even thinking about operations, and possibly memory, but just values manipulation), and makes the analysis simpler and more effective. In languages which are actually functional, this may empower the compiler to act more aggressively. In Python, it makes the linters more effective[*], leading to fewer errors and more helpful messages :)

[*]: since it optimizes the simplicity of the code, it makes it also more human-readable in general

It definitely makes sense, thank you!

alecandido

Generally good, and pretty much ready to go.

The main suggestions are about the docs. The other ones are very minor refactor requests, which could even be neglected.

alecandido · 2024-12-03T16:27:38Z

doc/source/protocols/flipping.rst

+	- id: flipping
+	  operation: flipping
+	  parameters:
+	    delta_amplitude: 0.05
+	    nflips_max: 30
+	    nflips_step: 1
+	    rx90: True


Try to indent with spaces, not tabs (you can set your editor to do that)

alecandido · 2024-12-03T16:27:47Z

doc/source/protocols/rabi/rabi.rst

+        pulse_length: 40
+        nshots: 1024
+        relaxation_time: 50000
+	rx90: True


Another tab here ^^

alecandido · 2024-12-03T16:32:02Z

doc/source/protocols/rabi/rabi.rst

+In all the previous examples we run Rabi experiments for calibrating the amplitude (duration) of the drive pulse
+to excite the qubit from the ground state up to state :math:`\ket{1}`.
+All these example runcards can be modified to calibrate the amplitude (duration) of the drive pulse
+to excite the qubit from the ground state up to state :math:`\frac{\ket{0}-i\ket{1}}{\sqrt{2}}` by simply setting the `rx90` parameter to `True`.


Instead of mentioning the experiment in the general section, and describe it only in the example, you may move this description up, and just cite here you're performing the pi/2 described above.

Moreover, a description of the sequence (i.e. "same, but replace each pi with two pi/2") and a brief motivation may be appreciated. Then, you may cite this section in the flipping, instead of repeating it.

Even a reference to the literature you consulted may be appreciated as well :)

alecandido · 2024-12-03T16:45:10Z

src/qibocal/protocols/flipping.py

+    if rx90:
+        sequence |= natives.RX90()

-        qd_channel, rx_pulse = natives.RX()[0]
+        for _ in range(flips):
+            qd_channel, qd_pulse = natives.RX90()[0]

-        rx_detuned = update.replace(
-            rx_pulse, amplitude=rx_pulse.amplitude + delta_amplitude
-        )
-        sequence.append((qd_channel, rx_detuned))
-        sequence.append((qd_channel, rx_detuned))
+            qd_detuned = update.replace(
+                qd_pulse, delta_amplitude=qd_pulse.amplitude + delta_amplitude
+            )
+
+            sequence.append((qd_channel, qd_detuned))
+            sequence.append((qd_channel, qd_detuned))
+            sequence.append((qd_channel, qd_detuned))
+            sequence.append((qd_channel, qd_detuned))
+
+        sequence |= natives.MZ()
+
+    else:
+        sequence |= natives.R(theta=np.pi / 2)

-    sequence |= natives.MZ()
+        for _ in range(flips):
+            qd_channel, qd_pulse = natives.RX()[0]
+
+            qd_detuned = update.replace(
+                qd_pulse, amplitude=qd_pulse.amplitude + delta_amplitude
+            )
+            sequence.append((qd_channel, qd_detuned))
+            sequence.append((qd_channel, qd_detuned))
+
+        sequence |= natives.MZ()


A good chunk of code is duplicated between the two branches. You may try to scope further the if within the sequence definition, to minimize duplication.

(In general, there is no exact solution about how to make code "minimal" or "simplest" - but here there is some room for improvements ^^)

alecandido · 2024-12-03T16:48:32Z

src/qibocal/protocols/flipping.py

@@ -222,10 +254,14 @@ def _fit(data: FlippingData) -> FlippingResults:
            perr = np.sqrt(np.diag(perr)).tolist()
            popt = popt.tolist()
            correction = popt[2] / 2
+
+            if data.rx90:
+                correction = correction / 2


In some other contexts, there is a difference between redefinition and in-place mutation, and the distinction may be very relevant.

However, in this case it's just about a single scalar, so it makes no difference (to the point that a compiler may optimize the operation anyhow). So, better to use the idiomatic shortcut ;)

Suggested change

correction = correction / 2

correction /= 2

alecandido · 2024-12-03T16:50:42Z

src/qibocal/protocols/flipping.py

+            qubit: (
+                platform.natives.single_qubit[qubit].RX90[0][1].amplitude
+                if params.rx90
+                else platform.natives.single_qubit[qubit].RX[0][1].amplitude
+            )


Ok, this is just for fun, and at the level of code golf. But just for you to know that such an option exists

Suggested change

qubit: (

platform.natives.single_qubit[qubit].RX90[0][1].amplitude

if params.rx90

else platform.natives.single_qubit[qubit].RX[0][1].amplitude

)

qubit: getattr(platform.natives.single_qubit[qubit], "RX90" if params.rx90 else "RX")[0][1].amplitude

alecandido · 2024-12-03T16:54:17Z

src/qibocal/protocols/rabi/utils.py

+        if rx90:
+            qd_channel, qd_pulse = natives.RX90()[0]
+        else:
+            qd_channel, qd_pulse = natives.RX()[0]


In case you like code golf as well, you may try to deduplicate even this one (but it's absolutely not needed)

alecandido · 2024-12-03T16:57:02Z

tests/test_protocols.py

+        extract_rabi(RabiAmplitudeEFData)
+
+
+def test_extract_rabi():


This is redefining the former, thus shadowing it.

You have to use two different names in order to be able to access both, and thus also to run both in Pytest.

While a redefinition is not an error, it is unlikely that you want to define a function that already exists. Consequently, your editor may have warned you about this (or better, Ruff should have done it).

ElStabilini added 2 commits November 19, 2024 11:19

first test for swittching from pi to pi-half

14f80b4

fixed pi half calibration sequence

ec4b7ad

ElStabilini requested review from alecandido, andrea-pasquale and Edoardo-Pedicillo November 20, 2024 13:07

andrea-pasquale changed the base branch from main to 0.2 November 20, 2024 13:08

modified classes and methods for running rabi for RX90

9f63c69

andrea-pasquale reviewed Nov 20, 2024

View reviewed changes

src/qibocal/update.py Outdated Show resolved Hide resolved

src/qibocal/protocols/rabi/length_signal.py Outdated Show resolved Hide resolved

src/qibocal/protocols/rabi/length_frequency_signal.py Outdated Show resolved Hide resolved

ElStabilini added 2 commits November 21, 2024 08:37

added option to save duration of RX90 pulse

33db217

started modifying rabi ef for using RX90

32cceda

ElStabilini and others added 10 commits November 21, 2024 11:56

modified pihalf_pulse to rx90 in parameters

7661361

Update rabi.rst

d03f383

[pre-commit.ci] auto fixes from pre-commit.com hooks

d0c1228

for more information, see https://pre-commit.ci

set pi calibration as default and change rx90 dtype

e6a2395

Saving modified documentation

144f468

Merge branch 'pi_half' of github.com:qiboteam/qibocal into pi_half

fix ordering error

c112748

moved all pihalf_pulses in Rabi-Data class

f083434

moved all pihalf_pulses in Rabi-Data class

f52765f

fixed errors

efcb55a

Update rabi.rst

66f0b71

We changed the default runcard option to False (by default will calibrate pi pulse)

andrea-pasquale reviewed Nov 21, 2024

View reviewed changes

andrea-pasquale changed the title ~~RX$\pi/2$ calibration implementation~~ RX90 calibration implementation Nov 21, 2024

ElStabilini added 2 commits November 21, 2024 18:17

fixed amplitude_sequence and length_sequence inputs

1f16f50

Sync remote and local branches

f8e9021

Merge branch 'pi_half' of github.com:qiboteam/qibocal into pi_half

fixed acquisition error

a08cf3c

ElStabilini and others added 7 commits November 27, 2024 19:33

Update flipping.py

f6bfcaf

Update flipping.py

454f5b5

Update flipping.py

47a22b5

Update flipping.py

96dd44e

Update flipping.py

07a4bcd

[pre-commit.ci] auto fixes from pre-commit.com hooks

e8cea9a

for more information, see https://pre-commit.ci

Update flipping.py

4052755

ElStabilini and others added 6 commits November 28, 2024 11:14

fix FlippingResults and FlippingData class definition

12fedb5

fix globalbackends error: downgrade qibo

b58e13a

Update flipping.rst

32308e2

[pre-commit.ci] auto fixes from pre-commit.com hooks

57904a1

for more information, see https://pre-commit.ci

fix error in _fit method

3b6769d

update documentation for flipping

29a1296

Merge branch 'pi_half' of github.com:qiboteam/qibocal into pi_half

Edoardo-Pedicillo added this to the Qibocal - Qibolab 0.2 milestone Nov 28, 2024

ElStabilini added 4 commits November 28, 2024 18:23

fix tests for rx90

022240d

Update flipping.rst

8b0995d

modify plot function in utils to have the correct name of the pulse c…

98bcf1d

…alibrated

'sync remote and local branch'

17ba606

Merge branch 'pi_half' of github.com:qiboteam/qibocal into pi_half

alecandido reviewed Nov 29, 2024

View reviewed changes

improve code for printing pulse names on reports

7b907ae

ElStabilini added 4 commits December 3, 2024 10:34

fix compatibility with qibo v0.2.14

7660fde

fix conflict with pyproject.toml for branch 0.2

2d8c8f7

fix error in flipping sequence construction for rx90

1bac594

Merge branch '0.2' into pi_half

af82fec

ElStabilini marked this pull request as ready for review December 3, 2024 15:31

ElStabilini requested a review from alecandido December 3, 2024 17:40

alecandido reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RX90 calibration implementation #1044

RX90 calibration implementation #1044

ElStabilini commented Nov 20, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading

ElStabilini commented Nov 20, 2024

andrea-pasquale commented Nov 20, 2024

ElStabilini commented Nov 21, 2024 •

edited

Loading

andrea-pasquale left a comment

andrea-pasquale Nov 21, 2024

ElStabilini Nov 22, 2024 •

edited by alecandido

Loading

andrea-pasquale Nov 21, 2024

ElStabilini Nov 22, 2024

andrea-pasquale commented Nov 21, 2024

alecandido commented Nov 27, 2024 •

edited

Loading

alecandido Nov 29, 2024

alecandido commented Nov 30, 2024

ElStabilini commented Nov 30, 2024

alecandido left a comment

alecandido Dec 3, 2024

alecandido Dec 3, 2024

alecandido Dec 3, 2024

alecandido Dec 3, 2024

alecandido Dec 3, 2024

alecandido Dec 3, 2024

alecandido Dec 3, 2024

alecandido Dec 3, 2024

alecandido Dec 3, 2024

alecandido Dec 3, 2024

		update.drive_12_amplitude(
		results.amplitude[target], results.pihalf_pulse, platform, target

RX90 calibration implementation #1044

Are you sure you want to change the base?

RX90 calibration implementation #1044

Conversation

ElStabilini commented Nov 20, 2024 • edited Loading

codecov bot commented Nov 20, 2024 • edited Loading

Codecov Report

ElStabilini commented Nov 20, 2024

andrea-pasquale commented Nov 20, 2024

ElStabilini commented Nov 21, 2024 • edited Loading

andrea-pasquale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElStabilini Nov 22, 2024 • edited by alecandido Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-pasquale commented Nov 21, 2024

alecandido commented Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

alecandido commented Nov 30, 2024

ElStabilini commented Nov 30, 2024

alecandido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElStabilini commented Nov 20, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading

ElStabilini commented Nov 21, 2024 •

edited

Loading

ElStabilini Nov 22, 2024 •

edited by alecandido

Loading

alecandido commented Nov 27, 2024 •

edited

Loading