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

Fix duration interpolated sweeper for QM #1115

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Fix duration interpolated sweeper for QM #1115

merged 3 commits into from
Dec 16, 2024

Conversation

stavros11
Copy link
Member

Using the Rabi length routine, we noticed that this sweeper works best when we upload the pulse with the minimum duration among the swept values. This is probably due to the inability to compress arbitrary waveforms in real time (see https://docs.quantum-machines.co/latest/docs/Guides/features/?h=interpo#dynamic-pulse-duration).

Using this branch, the Rabi length behaves consistently between 0.1 (http://login.qrccluster.com:9000/MutpaUWhRPa9XlYyhD8mYA==) and 0.2 (http://login.qrccluster.com:9000/xKil3FR_Qv-4El7gQHd3vQ==).

Note that the standard (non-interpolated) sweeper still behaves differently for this experiment, however for that case there is no 0.1 counterpart that we can compare to.

@stavros11 stavros11 requested a review from alecandido December 6, 2024 17:41
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 50.65%. Comparing base (80bd287) to head (ca35c73).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/qibolab/_core/instruments/qm/controller.py 0.00% 11 Missing ⚠️
...bolab/_core/instruments/qm/program/instructions.py 0.00% 6 Missing ⚠️
.../qibolab/_core/instruments/qm/program/arguments.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1115      +/-   ##
==========================================
- Coverage   50.87%   50.65%   -0.23%     
==========================================
  Files          63       63              
  Lines        2909     2922      +13     
==========================================
  Hits         1480     1480              
- Misses       1429     1442      +13     
Flag Coverage Δ
unittests 50.65% <0.00%> (-0.23%) ⬇️

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
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.

Ok, the only thing I may have to amend is that this is a further decision we're taking within the driver (i.e. taking the minimum length one), and this may have an effect on the results (it certainly has an effect, it may be sizeable).

So, we should start documenting even these choices in public docs, including them in dedicated pages (guides/manual), instead of just updating the function docstrings.

However, this is something which is fully missing in Qibolab, so I would not propose to do it in this PR (which is more a patch for a broken implementation of the interpolated sweeper). But we may consider opening an issue for this, even one specific to QM, and start collecting behaviors that we want to document as part of the docs.

@stavros11
Copy link
Member Author

Ok, the only thing I may have to amend is that this is a further decision we're taking within the driver (i.e. taking the minimum length one), and this may have an effect on the results (it certainly has an effect, it may be sizeable).

So, we should start documenting even these choices in public docs, including them in dedicated pages (guides/manual), instead of just updating the function docstrings.

However, this is something which is fully missing in Qibolab, so I would not propose to do it in this PR (which is more a patch for a broken implementation of the interpolated sweeper). But we may consider opening an issue for this, even one specific to QM, and start collecting behaviors that we want to document as part of the docs.

For now, I improved a bit the relevant docstring in ca35c73, including the link to QM docs. Regarding a larger doc refactoring, I agree that it should be in a different PR, so feel free to open an issue.

However, I am not sure how much this particular thing is worth documenting, because it is not really a choice, it is the only way the interpolated sweeper will work for arbitrary waveforms in QM (unless I am missing something). They even say in their docs that setting the real-time duration parameter in play below the original length of the uploaded waveform may lead to corrupt output (not even an error), so I do not see any other option.

@alecandido
Copy link
Member

However, I am not sure how much this particular thing is worth documenting, because it is not really a choice, it is the only way the interpolated sweeper will work for arbitrary waveforms in QM (unless I am missing something). They even say in their docs that setting the real-time duration parameter in play below the original length of the uploaded waveform may lead to corrupt output (not even an error), so I do not see any other option.

That is correct, in a sense.
If the only purpose of Qibolab were to standardize the interface, Qibolab users would be familiar with the devices and their API (not necessarily Qibocal's ones).

However, at least historically, Qibolab served the purpose to also abstract its users from the devices. Thus, it is effectively encapsulating the devices' behavior in the drivers.

I'd try to go towards a more transparent Qibolab, and that will be the purpose of #917 and related. But for as long as the average Qibolab's user is mostly unaware of the devices' specificities, it'd be better to at least propagate the pointers to the relevant docs in ours.

@stavros11 stavros11 merged commit cbaf2c6 into main Dec 16, 2024
27 of 28 checks passed
@stavros11 stavros11 deleted the interpolated branch December 16, 2024 13:09
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.

2 participants