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

Do not rearrange real-time sweepers (ZI) #852

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Do not rearrange real-time sweepers (ZI) #852

merged 1 commit into from
Mar 28, 2024

Conversation

hay-k
Copy link
Contributor

@hay-k hay-k commented Mar 25, 2024

Currently, the ZI driver may change the order of real-time sweepers in certain situations. This was most probably historically done for the following two reasons:

  1. hardware limitation(s)
  2. optimization of runtime (frequency sweep is considered to be slow, so it is moved to the outer loop, so it changes the least amount of time)

Recently, we discovered that (and confirmed by laboneq developers), that point 1 does not exist. As for point 2, with @Jacfomg we observed that order of real-time sweeps may affect the quality of acquired data (for physical reasons, and not specific to instrument). In light of this it seems that the better decision is that the driver not change the order of sweeps. It is better to let qibocal decide which order works best for which experiment.

Below is output from qubit flux dependence experiment, where flux sweep is done as an amplitude sweep for a long square flux pulse. So we have nested 2 real-time sweeps (flux pulse amplitude and qubit drive frequency). The first plot corresponds to the case where frequency sweep is the inner loop, and for the second plot the frequency sweep is the outer loop.

image image

Looks like the rapid changes in amplitude (when the amplitude sweep is the inner one) result into some vertical shadows. The two experiments above were done using relaxation_time = 2000. Increasing it mitigates the shadowing issue, as seen from the below plot done with relaxation_time=20_000:
image

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.66%. Comparing base (c109c51) to head (061aa18).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #852      +/-   ##
==========================================
+ Coverage   66.60%   66.66%   +0.05%     
==========================================
  Files          55       55              
  Lines        5917     5900      -17     
==========================================
- Hits         3941     3933       -8     
+ Misses       1976     1967       -9     
Flag Coverage Δ
unittests 66.66% <ø> (+0.05%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Jacfomg Jacfomg left a comment

Choose a reason for hiding this comment

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

Looks, great to me. However, I will wait a bit to merge it until qibolcal routines get the proper order.

@hay-k hay-k changed the title Do not rearrange sweepers (ZI) Do not rearrange real-time sweepers (ZI) Mar 25, 2024
Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

I agree that sweepers order should not change, in any case. This should be honored, both to give more control to the user (and avoid unexpected behavior), and to keep the driver simple and minimal (the first motivation is more important than the second, but they are both relevant and, in this case, pointing in the same direction).

@alecandido
Copy link
Member

alecandido commented Mar 25, 2024

Looks like the rapid changes in amplitude (when the amplitude sweep is the inner one) result into some vertical shadows.

I can understand that this is also solved by rearranging sweepers, but if there were other reasons to keep the second order, it should be also solved by a sufficient relaxation time. Isn't it?

@hay-k
Copy link
Contributor Author

hay-k commented Mar 25, 2024

should be also solved by a sufficient relaxation time. Isn't it?

Yes!

EDIT: added a plot related to that to the main description of the PR for completeness.

@Jacfomg
Copy link
Contributor

Jacfomg commented Mar 28, 2024

@hay-k rebased with the flux pulse hotfix so the issue I found is gone and we can merge it.

@hay-k hay-k merged commit 48d0352 into main Mar 28, 2024
24 checks passed
@hay-k hay-k deleted the no-rearranging branch March 28, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants