Skip to content

Update RB to reduce the size of the generated circuits #7411

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

Merged
merged 24 commits into from
Jul 9, 2025

Conversation

NoureldinYosri
Copy link
Collaborator

@NoureldinYosri NoureldinYosri commented Jun 4, 2025

the cirq RB sequences were often 2-3 times bigger than they should, this is because for historical reasons the cliffords were written as products of X/Z or X/Y gates. In this PR I merge these gates into a single PhasedXZGate which is the same as we do internally.

@NoureldinYosri NoureldinYosri requested review from mrwojtek, vtomole and a team as code owners June 4, 2025 20:39
@github-actions github-actions bot added the size: S 10< lines changed <50 label Jun 4, 2025
@github-actions github-actions bot added size: M 50< lines changed <250 and removed size: S 10< lines changed <50 labels Jun 4, 2025
Copy link

codecov bot commented Jun 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.68%. Comparing base (b3fe874) to head (722e907).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7411      +/-   ##
==========================================
- Coverage   98.68%   98.68%   -0.01%     
==========================================
  Files        1092     1092              
  Lines       96635    96672      +37     
==========================================
+ Hits        95366    95401      +35     
- Misses       1269     1271       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

Thanks, Nour!! I tested this, and it works!

Could we also change the default value for use_xy_basis in cirq.experiments.parallel_single_qubit_randomized_benchmarking to False, since that is the default value internally? Thanks!

@NoureldinYosri NoureldinYosri requested review from pavoljuhas and removed request for tanujkhattar June 5, 2025 16:40
@NoureldinYosri NoureldinYosri enabled auto-merge June 5, 2025 16:41
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.

Please replace _CliffordGateSequence with a simple conversion.

Otherwise LGTM.

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.

Before merging, see b/422636519

@github-actions github-actions bot added size: S 10< lines changed <50 and removed size: M 50< lines changed <250 labels Jun 26, 2025
@github-actions github-actions bot added size: M 50< lines changed <250 and removed size: S 10< lines changed <50 labels Jun 30, 2025
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 two small adjustments.

@github-actions github-actions bot added size: L 250< lines changed <1000 and removed size: M 50< lines changed <250 labels Jul 8, 2025
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.

Thanks, Nour! This looks a lot better!

I'm noticing that there is more instance-to-instance fluctuation in the error rates with this implementation than with the internal one. See this colab.

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 for the API, thanks Nour!

@NoureldinYosri
Copy link
Collaborator Author

@eliottrosenberg there are two sources of randomness when running RB, the first is intrinsic that comes from running the circuits on a quantum chip, the second is that everytime the method is run a new set of circuits is generated.

the old api which you are using the notebook didn't expose a way to control the circuit generation. the new one does. so if you call parallel_single_qubit_rb(..., rng_or_seed=0) you will match the internal std because currently the default value is None which means a new set of circuits for each run.

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.

Approving, but let's follow up to resolve the remaining discrepancies.

@NoureldinYosri NoureldinYosri added this pull request to the merge queue Jul 9, 2025
Merged via the queue into quantumlib:main with commit e42eac2 Jul 9, 2025
35 checks passed
@NoureldinYosri NoureldinYosri deleted the optimize_rb branch July 9, 2025 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants