-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Support multi moment gauge compiling #7501
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
base: main
Are you sure you want to change the base?
Conversation
c182892
to
cc2b38a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7501 +/- ##
========================================
Coverage 97.53% 97.54%
========================================
Files 1095 1098 +3
Lines 99016 99252 +236
========================================
+ Hits 96580 96815 +235
- Misses 2436 2437 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
32c5bb3
to
05cc48b
Compare
Hey @eliottrosenberg @NoureldinYosri , here is multi moment gauge compiling!! After evaluating multiple strategies, this approach proves to be optimal regarding both execution speed and future extensibility. Note gauges could be automatically added to circuits with default setup, wonder if the default setup is suitable for most use cases? (see Example section and Usage section in the description above) |
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! Great work, @babacry!
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.
@babacry is there a way to reuse code you wrote before in _PauliAndZPow
?
cirq-core/cirq/transformers/gauge_compiling/multi_moment_gauge_compiling.py
Outdated
Show resolved
Hide resolved
cirq-core/cirq/transformers/gauge_compiling/multi_moment_gauge_compiling.py
Outdated
Show resolved
Hide resolved
else: | ||
self.zpow = ops.ZPowGate(exponent=-left.exponent + self.zpow.exponent) | ||
|
||
def _merge_right_zpow(self, right: ops.ZPowGate): |
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.
didn't you implement the logic for pulling gates somewhere else? or is this specific to these gates?
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.
Not really, existing functions only allow for pulling Pauli gates through Clifford gates. However, the cphase gate is non-Clifford. When a Pauli operator is pushed through a cphase gate, it transforms into a combination of a Pauli operator and a Z-rotation. To handle this, the method tracks both the Pauli operator and the additional Z-rotation throughout the moments.
|
||
|
||
class CPhaseGaugeTransformerMM(MultiMomentGaugeTransformer): | ||
|
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.
lets make both MultiMomentGaugeTransformer
and CPhaseGaugeTransformerMM
be @attrs.frozen
then here we can do
supported_gates = attrs.field(default=_SUPPORTED_GATESET)
target = attrs.field(default=_TARGET_GATESET, init=False, repr=False, eq=False, hash=False)
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.
Frozen isn't very compatible with the level of flexibility we want to support, e.g., we want to allow the 3 use cases:
- default setup that allows gauging on extra supported gates other than cphase gates
- {required: cpahse, optional: _SUPPORTED_GATES}.
- only gauge on cphase-only moments.
- {required: cphase, optional: None}
- users can specify what gates they would want to gauge on except cphase gates, e.g., user might want to gauge on moments with
- {required: cphase, optional: X, Rz}.
Thus, I expose supported_gates in the init function of CPhaseGaugeTransformerMM.
See pr description for the first 2 use cases.
I am also okay with changing the user interface, what do you think?
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.
you can still support these cases with an attrs.frozen, let me know if you need help structuring it
Thanks Nour for the insightful reivews! We don't have a _PauliAndZPow support in cirq yet. If you are mentioning the PR I worked on to support a specific use case of multi moment gauge compiling, the pr is closed. The only pull-through support in cirq is pulling Pauli gates through Cliffords. Could you help take another look? I've resolved some comments by committing changes, also left some comments for further discussions for the use of @attrs.frozen. @NoureldinYosri |
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.
initial review of style ... I still haven't reviewed the logic of commuting gates
) | ||
|
||
|
||
class _PauliAndZPow: |
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.
lets make this a public attrs.frozen
|
||
|
||
class _PauliAndZPow: | ||
"""In pulling through, one qubit gate can be represented by a Pauli and an Rz gate. |
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.
"""In pulling through, one qubit gate can be represented by a Pauli and an Rz gate. | |
"""A gate represented by a Pauli followed by an Rz gate. | |
The order is --Pauli--ZPowGate--. | |
Attributes: | |
pauli: ... | |
zpow: ... | |
""" |
pauli: ops.Pauli | ops.IdentityGate = ops.I | ||
zpow: ops.ZPowGate = ops.ZPowGate(exponent=0) | ||
|
||
commuting_gates = {ops.I, ops.Z} # I,Z Commute with ZPowGate and CZPowGate; X,Y anti-commute. |
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.
static class members are weired in python, lets make this a private global variable _COMMUTING_GATES
|
||
commuting_gates = {ops.I, ops.Z} # I,Z Commute with ZPowGate and CZPowGate; X,Y anti-commute. | ||
|
||
def __init__( |
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.
you can drop this when you use @attrs.frozen
|
||
|
||
class CPhaseGaugeTransformerMM(MultiMomentGaugeTransformer): | ||
|
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.
you can still support these cases with an attrs.frozen, let me know if you need help structuring it
[ | ||
rng.choice( | ||
np.array([ops.I, ops.X, ops.Y, ops.Z], dtype=ops.Gate), | ||
p=[0.25, 0.25, 0.25, 0.25], |
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.
rng.choice is uniform by default so you can remove the p
parameter.
return circuits.Moment( | ||
[ | ||
rng.choice( | ||
np.array([ops.I, ops.X, ops.Y, ops.Z], dtype=ops.Gate), |
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 will create this array every time we call this method ... lets instead create a private global variable _PAULIS = ...
and here do _PAULIS[np.choice(4)]
] | ||
) | ||
|
||
def gauge_on_moments(self, moments_to_gauge) -> list[circuits.Moment]: |
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.
docstring?
q₃: ... ───LG3───╰───────────╯────RG3───... | ||
""" | ||
|
||
def __init__( |
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.
since this is an abstract class I don't think it needs an init
are permitted within the gauge moments. If a moment contains a gate not found | ||
in either target or supported_gates, it won't be gauged. | ||
""" | ||
self.target = ops.GateFamily(target) if isinstance(target, ops.Gate) else target |
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.
either let mypy know about the existance of these properities by just declaring them
class MultiMomentGaugeTransformer:
target: ops.GateFamily
supported_gates: ops.GateFamily
or by making abstract methods that return them
Support multi moment gauge transformation, where target moments will be gauged with new moments, e.g.,
MultiMomentGaugeTransformer
class is defined.CPhaseGaugeTransformerMM
.CPhaseGaugeTransformerMM
, e.g., Pauli Gates, ZPowGate etc. If these gates are detected, gauge will be calculated accordingly. Users can tweak their desired gates by instantiate their own transformer bytransformer = CPhaseGaugeTransformerMM(supported_gates=ops.Gateset(g1,g2,g3))
. By default_SUPPORTED_GATESET
are used.Example:
Usage
case 1: default setup, any moment with CZPowGate and optionally with _SUPPORTED_GATES will be gauged (see example above),
case 2: Only CZPowGate-only moments will be gauged (see
test_gauge_on_czpow_only_moments
test case),