Skip to content
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

Refactor ramsey to use detuning instead of number of oscillations #721

Merged
merged 12 commits into from
Feb 28, 2024

Conversation

andrea-pasquale
Copy link
Contributor

@andrea-pasquale andrea-pasquale commented Feb 19, 2024

Refactoring all Ramsey protocol to use directly a detuning parameter instead of the number of oscillations.
In this way the detuning is introduced directly and not artificially through the phase as already mentioned in #696.
I also tried to remove some code duplication.

I'm planning to do also the following:

  • Improve documentation
  • Check detuning sign

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

Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9c7a34c) 97.19% compared to head (518b397) 97.10%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #721      +/-   ##
==========================================
- Coverage   97.19%   97.10%   -0.10%     
==========================================
  Files         110      110              
  Lines        7632     7559      -73     
==========================================
- Hits         7418     7340      -78     
- Misses        214      219       +5     
Flag Coverage Δ
unittests 97.10% <100.00%> (-0.10%) ⬇️

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

Files Coverage Δ
src/qibocal/protocols/characterization/__init__.py 100.00% <100.00%> (ø)
...ibocal/protocols/characterization/ramsey/ramsey.py 99.28% <100.00%> (ø)
...protocols/characterization/ramsey/ramsey_signal.py 96.42% <100.00%> (ø)
...qibocal/protocols/characterization/ramsey/utils.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@@ -166,48 +152,18 @@ def _acquisition(
errors=errors,
),
)
if params.n_osc != 0:

if params.detuning != 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we still need a separate case for detuning != 0? If we are detuning the frequency, I think we can use the same sweeper in both cases. It is not like before that we had to sweep the phase together with the duration which was not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed when I was refactoring I was thinking that we can avoid having the two different cases.
Perhaps it could be useful to keep also a version with unrolling given that some instruments (qblox) have memory issues when dealing with duration sweepers. Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now both ramsey and ramsey detuned work with sweepers.
I kept the option for unrolling: if the user set unrolling=True the platform will unroll the sequences instead of using the sweeper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also decided to drop ramsey_sequences since everything should be now included in ramsey and ramsey_signal.

@igres26
Copy link
Contributor

igres26 commented Feb 20, 2024

Could you add some of the images that you get from running this one on the available chip. Detune around 1 MHz and with maximum duration of around 2000 ns. With this, we should also be able to detect the right direction of the detuning and correct it accordingly.

@igres26 igres26 self-requested a review February 20, 2024 12:34
@andrea-pasquale andrea-pasquale marked this pull request as ready for review February 20, 2024 14:32
@andrea-pasquale
Copy link
Contributor Author

The PR is now ready to review. I've performed some tests with both unrolling/sweepers and I've tested also that the correction for the drive frequency has the correct sign.

@andrea-pasquale andrea-pasquale added this pull request to the merge queue Feb 28, 2024
Merged via the queue into main with commit 61626e6 Feb 28, 2024
21 checks passed
@andrea-pasquale andrea-pasquale deleted the ramsey_detuning branch February 28, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants