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

Combine 2q parallel XEB into one methods to simplify the XEB workflow #6443

Merged
merged 14 commits into from
Feb 8, 2024

Conversation

NoureldinYosri
Copy link
Collaborator

@NoureldinYosri NoureldinYosri commented Feb 5, 2024

This way the notebook https://quantumai.google/cirq/noise/qcvv/parallel_xeb becomes

>>> res = cirq.experiments.parallel_two_qubit_xeb(sampler)
>>> res.plot_heatmap()

@CirqBot CirqBot added the size: M 50< lines changed <250 label Feb 5, 2024
@NoureldinYosri NoureldinYosri marked this pull request as ready for review February 6, 2024 23:33
@NoureldinYosri NoureldinYosri requested review from mrwojtek, vtomole, cduck and a team as code owners February 6, 2024 23:33
@NoureldinYosri NoureldinYosri requested a review from verult February 6, 2024 23:33
@NoureldinYosri
Copy link
Collaborator Author

@eliottrosenberg this is now ready for review

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (f2c6f3c) 97.81% compared to head (a91e175) 97.81%.
Report is 3 commits behind head on main.

❗ Current head a91e175 differs from pull request most recent head 64effd8. Consider uploading reports for the commit 64effd8 to get more accurate results

Files Patch % Lines
cirq-core/cirq/experiments/two_qubit_xeb.py 98.85% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a comment

Choose a reason for hiding this comment

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

@NoureldinYosri
Copy link
Collaborator Author

NoureldinYosri commented Feb 7, 2024

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

@eliottrosenberg eliottrosenberg self-requested a review February 7, 2024 01:44
Copy link
Collaborator

@eliottrosenberg eliottrosenberg left a 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 for cirq.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.

@NoureldinYosri
Copy link
Collaborator Author

@eliottrosenberg I added the histogram method and removed the print statements.

@NoureldinYosri NoureldinYosri enabled auto-merge (squash) February 7, 2024 22:35
@eliottrosenberg
Copy link
Collaborator

@eliottrosenberg I added the histogram method and removed the print statements.

Thanks, Nour!

@pavoljuhas pavoljuhas self-requested a review February 8, 2024 00:44
Comment on lines +96 to +98
res.plot_heatmap(ax=ax)
res.plot_fitted_exponential(cirq.GridQubit(4, 4), cirq.GridQubit(4, 3), ax=ax)
res.plot_histogram(ax=ax)
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

@pavoljuhas pavoljuhas left a 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.

@NoureldinYosri NoureldinYosri enabled auto-merge (squash) February 8, 2024 01:33
@NoureldinYosri NoureldinYosri changed the title Combine 2q parallel XEB into two methods, one for benchemarking and the other for visualization Combine 2q parallel XEB into one methods to simplify the XEB workflow Feb 8, 2024
@NoureldinYosri NoureldinYosri merged commit 34a8e59 into main Feb 8, 2024
35 checks passed
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants