Update RB to reduce the size of the generated circuits #7411
Conversation
github-actions
bot
added
size: M
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7411 +/- ##
==========================================
- Coverage 98.70% 98.69% -0.01%
==========================================
Files 1114 1114
Lines 98301 98312 +11
==========================================
+ Hits 97024 97028 +4
- Misses 1277 1284 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
NoureldinYosri
requested review from
pavoljuhas
and removed request for
tanujkhattar
| def parallel_single_qubit_randomized_benchmarking( | ||
| sampler: cirq.Sampler, | ||
| qubits: Sequence[cirq.Qid], | ||
| use_xy_basis: bool = True, | ||
| use_xy_basis: bool = False, | ||
| *, |
There was a problem hiding this comment.
What is the purpose of the "*," here?
There was a problem hiding this comment.
it forces those other parameters to be keyword only
There was a problem hiding this comment.
It doesn't make a lot of sense to have use_xy_basis be above the star, I think.
There was a problem hiding this comment.
I agree it would be cleaner, but converting use_xy_basis to a keyword-only argument would break any callers that pass it as a positional arg. It is probably not worth the inconvenience.
There was a problem hiding this comment.
I don't think there are many users of this RB implementation and I'm not worried about breaking old workflows here.
There was a problem hiding this comment.
I tested it in this colab, and it seems that the updated Cirq RB gives error rates that are higher than the internal RB.
| use_xy_basis: bool = True, | ||
| use_xy_basis: bool = False, |
There was a problem hiding this comment.
That interface has been around since #1533. Is there enough benefit from the new default to risk breaking any callers that expect the old value?
Also we have some callers of single_qubit_randomized_benchmarking and parallel_single_qubit_randomized_benchmarking in cirq sources that do not pass use_xy_basis.
Would they still work as intended?
I suggest to leave the default as is in this PR and if there is a strong argument for its change, do so in a separate PR (and label it as "BREAKING CHANGE").
There was a problem hiding this comment.
to me, it is fine either way. @eliottrosenberg what do you think?
There was a problem hiding this comment.
We should definitely change this (as Nour does here) in order to match the default behavior of the internal implementation of RB. I don't have a preference as to whether it is in this PR or a different one. @pavoljuhas I doubt that a lot of people are using this, so I'm not worried about breaking changes.
| def parallel_single_qubit_randomized_benchmarking( | ||
| sampler: cirq.Sampler, | ||
| qubits: Sequence[cirq.Qid], | ||
| use_xy_basis: bool = True, | ||
| use_xy_basis: bool = False, | ||
| *, |
There was a problem hiding this comment.
I agree it would be cleaner, but converting use_xy_basis to a keyword-only argument would break any callers that pass it as a positional arg. It is probably not worth the inconvenience.
Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
github-actions
bot
added
size: S
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.