-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Create a gauge for idle moments #7648
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
Conversation
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.
Awesome! Some small docstring changes
cirq-core/cirq/transformers/gauge_compiling/idle_moments_gague.py
Outdated
Show resolved
Hide resolved
cirq-core/cirq/transformers/gauge_compiling/idle_moments_gague.py
Outdated
Show resolved
Hide resolved
cirq-core/cirq/transformers/gauge_compiling/idle_moments_gague.py
Outdated
Show resolved
Hide resolved
cirq-core/cirq/transformers/gauge_compiling/idle_moments_gague.py
Outdated
Show resolved
Hide resolved
Awesome this looks really nice and useful |
great I will add tests and send for review |
Great, looking forward to having it in the codebase |
97059c3
to
408506d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7648 +/- ##
========================================
Coverage 99.37% 99.37%
========================================
Files 1078 1080 +2
Lines 96212 96378 +166
========================================
+ Hits 95609 95775 +166
Misses 603 603 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
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 think we can remove the gauges_inverse
and compute it from the gauges
list instead. Otherwise LGTM after addressing inline comments.
|
||
|
||
def _repr_fn(gauges: tuple[cirq.Gate, ...]) -> str: | ||
if gauges is _PAULIS: |
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.
Nit - here and below in case named gate tuples are created from a sequence.
if gauges is _PAULIS: | |
if gauges == _PAULIS: |
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 prefer to do both
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.
That makes a little difference for small tuples, but why not. :-)
There was a typo in variable name - fixed in 365e6c6
(in cached_property it is not worth to check identity)
cirq-core/cirq/transformers/gauge_compiling/idle_moments_gauge.py
Outdated
Show resolved
Hide resolved
cirq-core/cirq/transformers/gauge_compiling/idle_moments_gauge.py
Outdated
Show resolved
Hide resolved
cirq-core/cirq/transformers/gauge_compiling/idle_moments_gauge.py
Outdated
Show resolved
Hide resolved
cirq-core/cirq/transformers/gauge_compiling/idle_moments_gauge.py
Outdated
Show resolved
Hide resolved
cirq-core/cirq/transformers/gauge_compiling/idle_moments_gauge.py
Outdated
Show resolved
Hide resolved
tr = gc.IdleMomentsGauge(2, gauges='pauli') | ||
|
||
circuit = cirq.Circuit.from_moments([], [], [], cirq.X(cirq.q(0)), [], [], cirq.X(cirq.q(0))) | ||
transformed_circuit = tr(circuit, rng_or_seed=0) |
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.
Here we rely on np.random.default_rng(0).choice(4)
returning always the same value at
gate_index = rng.choice(len(self.gauges)) |
This can break if numpy generator changes in the future.
We could make this more secure by adding a local mock for rng.choice()
which would return preset values to get reproducible results. Your pick if it is worthwhile to do now.
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.
if a change in the generated cases breaks the test then the code is buggy ... the only reason I'm adding a seed is so that if it breaks we would know exactly which case it is
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.
actually you are right, I changed to using a mock
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 after pushing in eb2e865 to avoid overlap with the builtin gc module.
Thanks for adding this!
eb2e865
to
1863215
Compare
Adding a gauge compiler that surrounds empty sequences by randomly chosen gates ... this gauge is technically half way between a gauge and dynmical decoupling