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

Simplify Ramsey code #339

Merged
merged 3 commits into from
May 21, 2023
Merged

Simplify Ramsey code #339

merged 3 commits into from
May 21, 2023

Conversation

andrea-pasquale
Copy link
Contributor

Simplify the Ramsey code as suggested by @igres26 following #337.
This PR will not fix the bug for qblox as it is related to a problem with the relative phase.
I tried to test it this morning with qblox but I think that something is not working.
@Jacfomg could be please rerun quickly the sanity check that we ran yesterday using iqm?

Checklist:

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

@Jacfomg
Copy link
Contributor

Jacfomg commented May 18, 2023

Yes, I will do that. But also, we can take another look into why you could not manage to run things yesterday

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Merging #339 (bad2771) into main (7b3ad29) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #339      +/-   ##
==========================================
+ Coverage   57.95%   57.98%   +0.03%     
==========================================
  Files          67       67              
  Lines        4892     4896       +4     
==========================================
+ Hits         2835     2839       +4     
  Misses       2057     2057              
Flag Coverage Δ
unittests 57.98% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
src/qibocal/protocols/characterization/ramsey.py 95.38% <100.00%> (+0.14%) ⬆️

@andrea-pasquale
Copy link
Contributor Author

I think that it was because I had messed things up with conda.
I will give it another try.

@Jacfomg
Copy link
Contributor

Jacfomg commented May 18, 2023

This is with this code, @igres26 I also substracted .5 MHz from the qubit frequency and now you see the delta being negative but the correction is still working as intended as we substract it as well

image

image

Copy link
Contributor

@igres26 igres26 left a comment

Choose a reason for hiding this comment

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

Looks good. We could make it more clear that the sign in the (-2*np.pi) is the same as the -int((delta_fitting.... Maybe in the comments. But this works.

@andrea-pasquale
Copy link
Contributor Author

You are right.
If you want we could also add parameter detuning sign and we can provide a default value.
Another option could be to allow for negative values of n_osc. In this way we should be able to track the sign.
Let me know what do you think @igres26.

@andrea-pasquale
Copy link
Contributor Author

Tested and it works both with qblox (commenting a single line) and with RFSoC

@andrea-pasquale andrea-pasquale added this pull request to the merge queue May 21, 2023
Merged via the queue into main with commit 9418a98 May 21, 2023
@andrea-pasquale andrea-pasquale deleted the fix_delta_sign branch May 21, 2023 08:38
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