-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Combine 2q parallel XEB into one methods to simplify the XEB workflow #6443
Conversation
@eliottrosenberg this is now ready for review |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6443 +/- ##
========================================
Coverage 97.81% 97.81%
========================================
Files 1111 1113 +2
Lines 97198 97326 +128
========================================
+ Hits 95078 95204 +126
- Misses 2120 2122 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and got an error:
https://colab.research.google.com/drive/1mPHLGtMnh2l602Pqh9cgh9wpdEnvYHaB?usp=sharing
@eliottrosenberg please try again. I ran it several times and didn't run into this issue , though it can happen because of the random nature of operations. I assumed that qubit pairs are always ordered which turns out not to be true. I added a line to ensure results are accessed correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
A couple of nits:
- It would be nice to have a helper method that feeds the xeb errors into
cirq.integrated_histogram()
or returns a dictionary of all the error rates that could then be used as the input forcirq.integrated_histogram
. - The strings that it prints out, "Generate circuit library", "Generate random two qubit combinations", "Compute fidelities", and "Fit exponential decays," aren't really needed. They're useful for debugging, but now that it is working, I don't think they are needed.
@eliottrosenberg I added the histogram method and removed the print statements. |
Thanks, Nour! |
res.plot_heatmap(ax=ax) | ||
res.plot_fitted_exponential(cirq.GridQubit(4, 4), cirq.GridQubit(4, 3), ax=ax) | ||
res.plot_histogram(ax=ax) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem to check if plot functions have any effect - perhaps we can add simple assertions once they return axes?
I am also getting some UserWarning-s in the pytest output such as below which we could avoid -
.../quantumlib/Cirq/cirq-core/cirq/experiments/two_qubit_xeb.py:88: UserWarning: FigureCanvasAgg is non-interactive, and thus cannot be shown
fig.show()
Let's use @unittest.mock.patch("matplotlib.backend_bases.FigureCanvasBase.show"
for the plotting tests which will avoid user warning and it would be also possible to check if the mocked show()
got called as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't work. that method doesn't seem to exist. I can't find this being done in other places in Cirq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will take a look later if warnings can be stopped in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few tweaks for plot functions.
…he other for visualization (quantumlib#6443)
This way the notebook https://quantumai.google/cirq/noise/qcvv/parallel_xeb becomes