Ramsey unrolling #666

Jacfomg · 2023-12-11T13:38:08Z

It adds unrolling to the detuned ramsey

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 · 2023-12-11T13:44:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e9d484c) 97.32% compared to head (9a0f1e7) 97.29%.
Report is 61 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #666      +/-   ##
==========================================
- Coverage   97.32%   97.29%   -0.03%     
==========================================
  Files         106      106              
  Lines        7057     7108      +51     
==========================================
+ Hits         6868     6916      +48     
- Misses        189      192       +3

Flag	Coverage Δ
unittests	`97.29% <100.00%> (-0.03%)`	⬇️

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

Files	Coverage Δ
src/qibocal/protocols/characterization/ramsey.py	`100.00% <100.00%> (ø)`
...ibocal/protocols/characterization/ramsey_signal.py	`97.82% <100.00%> (+0.45%)`	⬆️

... and 1 file with indirect coverage changes

hay-k · 2023-12-21T06:04:10Z

src/qibocal/protocols/characterization/ramsey.py

@@ -161,31 +166,67 @@ def _acquisition(
                    errors=errors,
                ),
            )
-    else:
+    if params.n_osc != 0:


There is a lot of repeated code under the cases n_osc == 0 and n_osc != 0. I would like to suggest unifying them, or moving the repeated functionality into a function. It would be even nicer if the ramsey_signal also benefits from such unification. Unfortunately I am not able to understand what is the meaning of n_osc from its documentation, so cannot suggest a more concrete refactoring. Let's discuss.

Thanks @hay-k. We should definitely find a way to improve it. Just for reference n_osc stands for number of oscillations. In the ramsey protocol by putting a non zero integer value we include a detuning through the relative phase that will generate n_osc number of oscillations in the plot.

qibocal/src/qibocal/protocols/characterization/ramsey.py

Lines 190 to 195 in 528c3b9

RX90_pulses2[qubit].relative_phase = (

RX90_pulses2[qubit].start

* (-2 * np.pi)

* (params.n_osc)

/ params.delay_between_pulses_end

)

Having said this, I believe that in ramsey we differentiate between the n_osc==0 and the n_osc!=0 case because in the first case the protocol can be implemented through a simple sweeper on the start of the second RX90 pulse while in the second case, if I remember correctly, there are some limitations in our drivers that prevents us from sweeping start and relative phase at the same time. @stavros11 feel free to correct me if I said something wrong.

Of course the best solution for qibocal should be to keep the code as clean as possible and let the drivers handle these specific cases.

I see, thanks for the explanation! I think, in general, the drivers (and their developers, of course) will benefit a lot from a more unified and consistent implementation. Anyways, that's a story for another day. I believe we should still simplify the code in this particular instance, even without improving anything else anywhere else. That should be doable, and I am here to assist. @Jacfomg probably best if we discuss AFK.

Sure, it's sounds like a good plan.

hay-k · 2023-12-21T06:17:07Z

I don't see any unit tests. (in #665 also)

andrea-pasquale

I have not tested the code yet.
It seems ok. @Jacfomg you removed the ramsey_sequences because we are assuming that all the drivers will be compatible either with sweepers or with sequence unrolling, correct?

andrea-pasquale · 2023-12-21T07:13:36Z

tests/runcards/protocols.yml

+  - id: ramsey_signal_detuned
    priority: 0
-    operation: ramsey_sequences
+    operation: ramsey_signal
    parameters:
+      unrolling: True


@hay-k this is the uni test. In this yaml we put all the protocols that are later tested in tests/test_protocols.

hmm, let's discuss this, because if I understand those tests correctly, they are more like integration or end-to-end tests, rather than unit tests. But in any way this seems like a discussion that should be kept away from this PR, not to hinder its progress.

andrea-pasquale · 2023-12-21T07:21:52Z

src/qibocal/protocols/characterization/ramsey.py

@@ -161,31 +166,67 @@ def _acquisition(
                    errors=errors,
                ),
            )
-    else:
+    if params.n_osc != 0:


Thanks @hay-k. We should definitely find a way to improve it. Just for reference n_osc stands for number of oscillations. In the ramsey protocol by putting a non zero integer value we include a detuning through the relative phase that will generate n_osc number of oscillations in the plot.

qibocal/src/qibocal/protocols/characterization/ramsey.py

Lines 190 to 195 in 528c3b9

RX90_pulses2[qubit].relative_phase = (

RX90_pulses2[qubit].start

* (-2 * np.pi)

* (params.n_osc)

/ params.delay_between_pulses_end

)

Having said this, I believe that in ramsey we differentiate between the n_osc==0 and the n_osc!=0 case because in the first case the protocol can be implemented through a simple sweeper on the start of the second RX90 pulse while in the second case, if I remember correctly, there are some limitations in our drivers that prevents us from sweeping start and relative phase at the same time. @stavros11 feel free to correct me if I said something wrong.

Of course the best solution for qibocal should be to keep the code as clean as possible and let the drivers handle these specific cases.

Jacfomg · 2023-12-21T08:30:23Z

I have not tested the code yet. It seems ok. @Jacfomg you removed the ramsey_sequences because we are assuming that all the drivers will be compatible either with sweepers or with sequence unrolling, correct?

Yeah, I would say it is a fair assumption. It has been there for far too long and I'm not aware of it being used.

tests/runcards/protocols.yml

…into ramsye_unrolling

andrea-pasquale · 2024-02-01T10:19:35Z

tests/runcards/protocols.yml

    priority: 0
-    operation: ramsey_sequences
+    operation: ramsey_seqeunces


Suggested change

operation: ramsey_seqeunces

operation: ramsey_sequences

src/qibocal/protocols/characterization/__init__.py

Jacfomg added 2 commits December 11, 2023 17:29

draft

f5eff74

it was already working

84c2c33

Jacfomg requested review from andrea-pasquale and Edoardo-Pedicillo December 11, 2023 13:38

Jacfomg added 2 commits December 11, 2023 17:47

signal ramsey and remove useless ramsey file

4643ca0

fix

528c3b9

Jacfomg requested a review from stavros11 December 13, 2023 05:52

andrea-pasquale mentioned this pull request Dec 18, 2023

Adding ramsey sequences signal protocol #674

Closed

8 tasks

andrea-pasquale added this to the Qibocal 0.0.7 milestone Dec 18, 2023

hay-k reviewed Dec 21, 2023

View reviewed changes

andrea-pasquale reviewed Dec 21, 2023

View reviewed changes

Merge branch 'main' into ramsye_unrolling

577dc98

andrea-pasquale added the calibration label Jan 12, 2024

andrea-pasquale self-requested a review January 16, 2024 13:08

Merge branch 'main' into ramsye_unrolling

d23cb1a

stavros11 approved these changes Jan 31, 2024

View reviewed changes

andrea-pasquale reviewed Feb 1, 2024

View reviewed changes

tests/runcards/protocols.yml Show resolved Hide resolved

Jacfomg added 2 commits February 1, 2024 12:38

Add Ramsey sequences for detuned experiments

ac5eda7

Merge branch 'ramsye_unrolling' of https://github.com/qiboteam/qibocal …

a5b2f92

…into ramsye_unrolling

andrea-pasquale reviewed Feb 1, 2024

View reviewed changes

src/qibocal/protocols/characterization/__init__.py Outdated Show resolved Hide resolved

andrea-pasquale added 2 commits February 1, 2024 14:24

fix: Fix typo in tests and operations

4494d97

test: Fix coverage

9a0f1e7

andrea-pasquale approved these changes Feb 1, 2024

View reviewed changes

andrea-pasquale added this pull request to the merge queue Feb 1, 2024

Merged via the queue into main with commit 687a796 Feb 1, 2024
21 checks passed

andrea-pasquale deleted the ramsye_unrolling branch February 1, 2024 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ramsey unrolling #666

Ramsey unrolling #666

Jacfomg commented Dec 11, 2023

codecov bot commented Dec 11, 2023 •

edited

Loading

hay-k Dec 21, 2023

andrea-pasquale Dec 21, 2023

hay-k Dec 21, 2023

Jacfomg Dec 21, 2023

hay-k commented Dec 21, 2023

andrea-pasquale left a comment

andrea-pasquale Dec 21, 2023

hay-k Dec 21, 2023

andrea-pasquale Dec 21, 2023

Jacfomg commented Dec 21, 2023

andrea-pasquale Feb 1, 2024

	RX90_pulses2[qubit].relative_phase = (
	RX90_pulses2[qubit].start
	* (-2 * np.pi)
	* (params.n_osc)
	/ params.delay_between_pulses_end
	)

Ramsey unrolling #666

Ramsey unrolling #666

Conversation

Jacfomg commented Dec 11, 2023

codecov bot commented Dec 11, 2023 • edited Loading

Codecov Report

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

hay-k commented Dec 21, 2023

andrea-pasquale 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

Jacfomg commented Dec 21, 2023

Choose a reason for hiding this comment

codecov bot commented Dec 11, 2023 •

edited

Loading