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 bugs related to plotting #269

Merged
merged 7 commits into from
Mar 10, 2023
Merged

Fix bugs related to plotting #269

merged 7 commits into from
Mar 10, 2023

Conversation

andrea-pasquale
Copy link
Contributor

@andrea-pasquale andrea-pasquale commented Mar 8, 2023

Closes #266.

Checklist:

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

@andrea-pasquale andrea-pasquale added the bug Something isn't working label Mar 8, 2023
@andrea-pasquale andrea-pasquale added this to the Qibocal 0.0.2 milestone Mar 8, 2023
@andrea-pasquale andrea-pasquale self-assigned this Mar 8, 2023
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #269 (b55bac8) into main (b48a3f5) will decrease coverage by 0.32%.
The diff coverage is 52.63%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #269      +/-   ##
==========================================
- Coverage   36.26%   35.95%   -0.32%     
==========================================
  Files          19       19              
  Lines        1456     1452       -4     
==========================================
- Hits          528      522       -6     
- Misses        928      930       +2     
Flag Coverage Δ
unittests 35.95% <52.63%> (-0.32%) ⬇️

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

Impacted Files Coverage Δ
src/qibocal/plots/allXY.py 10.98% <ø> (ø)
src/qibocal/plots/flipping.py 15.90% <ø> (ø)
src/qibocal/plots/rabi.py 5.97% <ø> (ø)
src/qibocal/plots/ramsey.py 15.90% <ø> (ø)
src/qibocal/plots/spectroscopies.py 4.71% <0.00%> (-0.04%) ⬇️
src/qibocal/plots/spin_echo.py 15.90% <ø> (ø)
src/qibocal/plots/t1.py 15.90% <ø> (ø)
src/qibocal/fitting/methods.py 71.42% <90.90%> (-0.59%) ⬇️

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 fixing these issues. I tested after merging this to qmsweepers branch and spectroscopies, Rabi amplitude and length and allXY work for me.

For Ramsey I am still getting an error, now in builders.py update_platform_runcard line 282, that int cannot convert 1D array. It may be related to the fact that fitting failed and fits.csv does not have data for all qubits. It may be good to make this robust to fitting failures, because Ramsey fit fails quite often but we would still like to get the report.

I am now trying T1.

EDIT: More specifically what I have in mind is that update_platform_runcard checks if fitting data exist for the qubit. If yes it proceeds as now, otherwise it could print a warning like "Fitting data for qubit {qubit} not found. Runcard was not updated." and proceed with the code without failing.

EDIT2: T1 seems to work too. So far the issue remains only for Ramsey.

@andrea-pasquale
Copy link
Contributor Author

For Ramsey I am still getting an error, now in builders.py update_platform_runcard line 282, that int cannot convert 1D array. It may be related to the fact that fitting failed and fits.csv does not have data for all qubits. It may be good to make this robust to fitting failures, because Ramsey fit fails quite often but we would still like to get the report.

@stavros11 you mean ramsey_frequency_detuned, right?
The routine is pretty complicated and not optimized for multiplex I think.

@stavros11
Copy link
Member

stavros11 commented Mar 10, 2023

@stavros11 you mean ramsey_frequency_detuned, right? The routine is pretty complicated and not optimized for multiplex I think.

Sorry, I just saw this comment, I replied to the issue first. No, I am only using the normal ramsey, I have never used the detuned one. Even from before I remember that the fit for ramsey rarely works and even when it does the fitted curve is completely off the real one.

except:
log.warning("ramsey_fit: the fitting was not succesful")
data_fit.add({key: 0 for key in data_fit.df.columns})
Copy link
Contributor Author

@andrea-pasquale andrea-pasquale Mar 10, 2023

Choose a reason for hiding this comment

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

This was the bug. For some reasons, when the fitting fails we were setting all the keys to 0 (including the qubit key). This can be very dangerous since you could have two columns with the same qubit.

@andrea-pasquale
Copy link
Contributor Author

Thanks for the clarification.
I was able to spot the bug (see my comment).
Let me know if it works now @stavros11.

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 the update, it appears to work now. Ramsey fit still fails for most qubits but the report is still generated.

@andrea-pasquale
Copy link
Contributor Author

Coverage is not passing because we are missing some tests for fitting methods.
Given that this is out of the scope of this PR I suggest to merge this now.
In the meantime I will open an issue for the missing tests.

@andrea-pasquale andrea-pasquale merged commit 0577e91 into main Mar 10, 2023
@andrea-pasquale andrea-pasquale deleted the fix_bugs branch March 10, 2023 13:49
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
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Issues found while calibrating with QM
2 participants