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

Add fit to Chevron #520

Merged
merged 12 commits into from
Sep 25, 2023
Merged

Add fit to Chevron #520

merged 12 commits into from
Sep 25, 2023

Conversation

andrea-pasquale
Copy link
Contributor

@andrea-pasquale andrea-pasquale commented Sep 14, 2023

This PR implements a post-processing operation for the chevron protocol in order to find the best point for the CZ.

The post-processing works as follows:

  1. Perform FFT for each amplitude value
  2. Find amplitude at which the freq has the minimum value
  3. Compute duration by fitting cosinusoidal function at the point of minimum freq

This is the result on hardware
image

TODO:

  • Add table

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
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #520 (4077637) into main (6e5b3cf) will increase coverage by 0.04%.
Report is 9 commits behind head on main.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #520      +/-   ##
==========================================
+ Coverage   96.42%   96.47%   +0.04%     
==========================================
  Files          66       66              
  Lines        4789     4846      +57     
==========================================
+ Hits         4618     4675      +57     
  Misses        171      171              
Flag Coverage Δ
unittests 96.47% <100.00%> (+0.04%) ⬆️

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

Files Changed Coverage
.../characterization/two_qubit_interaction/chevron.py 100.00%
...racterization/two_qubit_interaction/cz_virtualz.py 100.00%
...ls/characterization/two_qubit_interaction/utils.py 100.00%

@andrea-pasquale andrea-pasquale changed the base branch from main to report_from_file September 14, 2023 09:18
@andrea-pasquale andrea-pasquale marked this pull request as draft September 14, 2023 10:09
@andrea-pasquale andrea-pasquale self-assigned this Sep 14, 2023
Base automatically changed from report_from_file to main September 15, 2023 13:54
@andrea-pasquale andrea-pasquale mentioned this pull request Sep 17, 2023
18 tasks
@andrea-pasquale andrea-pasquale added do not merge Soft warning to avoid merging a PR for reason provided in the PR qibolab release required PR requires qibolab feature not available in the current poetry.lock labels Sep 18, 2023
@andrea-pasquale andrea-pasquale marked this pull request as ready for review September 18, 2023 08:47
@andrea-pasquale andrea-pasquale changed the title Add fit to Chevron plot Add fit to Chevron and update mechanism for two qubit gates Sep 18, 2023
@andrea-pasquale andrea-pasquale removed do not merge Soft warning to avoid merging a PR for reason provided in the PR qibolab release required PR requires qibolab feature not available in the current poetry.lock labels Sep 20, 2023
@andrea-pasquale andrea-pasquale changed the title Add fit to Chevron and update mechanism for two qubit gates Add fit to Chevron Sep 20, 2023
@igres26
Copy link
Contributor

igres26 commented Sep 21, 2023

That is a good choice for fitting. More usage might expose where it fails, but for that we might want it merged.

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.

Of course, the critical part is the fit (as for the name of the PR), and we might need to iterate a bit.

All the other modifications are fine :)

Comment on lines 18 to 21
def fit_flux_amplitude(matrix, amps, times):
"""Estimate amplitude of Chevron plot by computing
FFT for each amplitude and chooses the amplitude
which lowers the frequency.
Copy link
Member

Choose a reason for hiding this comment

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

I know it's annoying, but the algorithm is definitely not trivial: it would be nice to have a complete description in English (rather than Python) of the whole algorithm.

Maybe we could discuss it before going through the process. Or you can use me as a guinea pig to test the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to have a look now.

Copy link
Member

@stavros11 stavros11 left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this. As a test I tried it on the chevron we got with QM on qw5q_gold last February (one of the first in TII) and it gave: amplitude=0.998, duration=28.7 which is acceptable. Here is the final curve fit in the amplitude slice (done quickly in a notebook):

curve_fit

We should address @alecandido's comments related to code, such as the redundant qubit variables, before merging, but I agree with @igres26 that in order to evaluate how good the fitting is we need to try it in practice. So I would not block this PR for this, particularly because now we don't have any QPUs to check. We can always improve the fitting later, as we did with other routines.

@andrea-pasquale
Copy link
Contributor Author

@alecandido I addressed all your comments but the improvements on the median for which I opened #535.
If everything else looks good for you I will merge soon given that I already have 2 approvs

@andrea-pasquale andrea-pasquale added this pull request to the merge queue Sep 25, 2023
Merged via the queue into main with commit c33d10f Sep 25, 2023
20 checks passed
@andrea-pasquale andrea-pasquale deleted the chevron_fit branch September 25, 2023 11:02
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, since we postponed the fit improvements, the rest is pretty much innocuous, and the implementation is fine.

Let's merge :)

@andrea-pasquale
Copy link
Contributor Author

Ok, since we postponed the fit improvements, the rest is pretty much innocuous, and the implementation is fine.

Let's merge :)

I saw that you approved the comment above so I merged...
Thanks for the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants