Introduce gauge compilation #6526

NoureldinYosri · 2024-03-26T18:45:53Z

This PR introduces the abstraction for Gauge compilation as well as implementation for Sycamore gate, CZ gate, SqrtCZ gate, ZZ (a.k.a spin inversion), ISWAP gate, and SQRT_ISWAP gate

codecov · 2024-03-26T19:02:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.79%. Comparing base (45c5fa3) to head (87ab1bb).

❗ Current head 87ab1bb differs from pull request most recent head db24b44. Consider uploading reports for the commit db24b44 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6526    +/-   ##
========================================
  Coverage   97.78%   97.79%            
========================================
  Files        1107     1124    +17     
  Lines       95188    95460   +272     
========================================
+ Hits        93079    93351   +272     
  Misses       2109     2109

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

eliottrosenberg

LGTM! (already tested and gave feedback on an earlier version via email)

Thanks for this great work, Nour!

cirq-core/cirq/transformers/gauge_compiling/iswap_gauge.py

cirq-core/cirq/transformers/gauge_compiling/sqrt_iswap_gauge.py

eliottrosenberg

For the sqrt_iswap gauges, since the gauges that you found are special cases of the continuous ones, I'm not sure it makes sense to include both. Maybe just include the continuous ones?

NoureldinYosri · 2024-03-29T23:14:53Z

For the sqrt_iswap gauges, since the gauges that you found are special cases of the continuous ones, I'm not sure it makes sense to include both. Maybe just include the continuous ones?

done

pavoljuhas · 2024-03-29T23:45:18Z

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py

+    pre_q0: _SINGLE_QUBIT_GATES_T = None
+    pre_q1: _SINGLE_QUBIT_GATES_T = None
+    post_q0: _SINGLE_QUBIT_GATES_T = None
+    post_q1: _SINGLE_QUBIT_GATES_T = None


For the sake of type simplicity - can this be just a tuple[ops.Gate, ...] with a () default instead?

Alternatively you could use Sequence[ops.Gate], but those arguments should be converted to tuples on initialization to make them immutable. This would let you also remove the _as_sequence conversion.

Union[ops.Gate, Tuple[ops.Gate, ...]] is more like what I want since I want users to be assign just a gate to these fields.

those arguments should be converted to tuples on initialization

The class is defined as dataclass(froze=True) and dataclasses library doesn't have field transformers like (unlike attrs), unless I use object.__setattr__ in __post_init, wdyt?

I think it would be better to require either the (a) tuple[ops.Gate, ...] or (b) Sequence[ops.Gate, ...] type for the pre_q / post_q fields. It is unlikely users would be defining these classes interactively; in such case it is OK to have them provide the sequence type that is stored in the instance.

For option (b) I am fine with using object.__setattr__ in __post_init to convert a sequence argument to tuple.
If that feels ugly, I am also fine with adding a dependency on attrs and using attrs.frozen with a field converter here.

Converting to a tuple on initialization would let you remove the _as_sequence helper and property caching which effectively store gate sequences twice.

used attrs with a field(converter=)

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py

pavoljuhas · 2024-03-30T00:31:57Z

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py

+                    q0, q1 = op.qubits
+                    left.extend([g(q) for g in gs] for q, gs in zip(op.qubits, gauge.pre))
+                    center.append(gauge.two_qubit_gate(q0, q1))
+                    right.extend([g(q) for g in gs] for q, gs in zip(op.qubits, gauge.post))


IIUC, the order of pre and post gates from different operations in the moment does not matter, correct?

Otherwise I would expect to add the post gates in a reversed order to pre-gates, e.g., [PreA, PreB, A, B, PostB, PostA].

yes, the order doesn't matter since the pre and post operations are single qubit ops. there is only one two-qubit gate here and that this the gauge.two_qubit_gate. so it doesn't matter if we do postA, postB or postB, postA

pavoljuhas · 2024-03-30T00:32:46Z

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py

+    The Gauge class in general represents a family of closely related gauges
+    (e.g. random z-rotations); Use `sample` method to get a specific gauge.
+    """
+


Maybe we can add annotation to make it clear derived classes should have that attribute.

Suggested change

two_qubit_gate: ops.Gate

not all derived classes have it. only ConstantGauge have it.

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling_test.py

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling_test_utils.py

cirq-core/cirq/transformers/gauge_compiling/iswap_gauge.py

pavoljuhas

Please see the inline comments. Otherwise LGTM.

pavoljuhas · 2024-04-01T22:10:55Z

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py

+
+
+def _select(choices: Sequence[Gauge], probabilites: np.ndarray, prng: np.random.Generator) -> Gauge:
+    return choices[prng.choice(len(choices), p=probabilites)]


The _select is only used here - consider moving that line to GaugeSelector.__call__.

it's needed here in order to force select specific gauges in tests

pavoljuhas · 2024-04-01T22:14:49Z

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py

+
+    def weight(self) -> float:
+        """Returns the relative frequency for selecting this gauge."""
+        return 1.0


The weight seems to be only used in the GaugeSelector.
How about moving weights to a field of GaugeSelector?
This would allow reuse of Gauge classes in several selectors with a different weights.

no the weight is a property of the gauge, the GaugeSelector just asks each gauge about its weights and normalizes them to get probabilities. but the weight is a property of a gauge.

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling_test_utils.py

pavoljuhas · 2024-04-01T22:39:14Z

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling_test_utils.py

+    gauge_transformer: GaugeTransformer
+
+    @pytest.mark.parametrize('generation_seed', [*range(5)])
+    @pytest.mark.parametrize('transformation_seed', [*range(5)])


The 2 parametrize decorators do Cartesian product so there is 25 test cases - is that necessary?
Perhaps we can combine to a single parametrization of 2 seed values.
Also consider using np.random.randint(2**31, size=(5, 2)).tolist() for the seed values in parametrization to check with non-repetitive seeding.

done, but instead of using np.random.randint I used np.random.RandomState(0).randint inorder to make the test reproducible

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling_test_utils_test.py

pavoljuhas · 2024-04-01T22:47:44Z

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling_test_utils_test.py

+@pytest.mark.xfail(strict=True)
+class TestInvalidTransformer(GaugeTester):
+    two_qubit_gate = _TEST_TARGET
+    gauge_transformer = _BAD_TRANSFORMER


This adds quite a few xfailed outcomes to pytest. Here the xfail is required, but in general it marks tests that are for features not yet working. Please consider removing this.

I replaced xfail with a flag

cirq-google/cirq_google/transformers/sycamore_gauge.py

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py

eliottrosenberg

LGTM! Thanks, Nour!

pavoljuhas

LGTM with 2 comments.

pavoljuhas · 2024-04-03T00:15:01Z

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py

+    def sample(self, gate: ops.Gate, prng: np.random.Generator) -> "ConstantGauge":
+        return self
+
+    def pre(self) -> Tuple[Tuple[ops.Gate, ...], Tuple[ops.Gate, ...]]:


I meant using a regular @property decorator instead of the cached_property.
Sorry about constant nitpicking - just that you can have your initial interface here if you want. :-)

pavoljuhas · 2024-04-03T00:30:03Z

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling_test_utils.py

+            if self.must_fail:
+                with pytest.raises(AssertionError):
+                    _check_equivalent_with_error_message(c, nc, gauge)
+            else:
+                _check_equivalent_with_error_message(c, nc, gauge)


Please consider removing the must_fail logic for the sake of simplicity and for whoever is going to read this in the future.

We don't check all assertions in tests to make sure they fail for bad tested functions. I feel it is sufficient that you verified your test class catches a bad transformer in an intermediate commit, but it is probably better to have a simpler rather than all covering final code here.

I disagree, without this the test module morph into something that accepts faulty gauges. this is essential for the integrity of the tests.

so either using the flag or xfail is need. for now I will keep the flag and we can revisit this later.

This PR introduces the abstraction for Gauge compilation as well as implementation for Sycamore gate, CZ gate, SqrtCZ gate, ZZ (a.k.a spin inversion), ISWAP gate, and SQRT_ISWAP gate

Introduce gauge compilation

7a5eecb

NoureldinYosri requested a review from eliottrosenberg March 26, 2024 18:47

NoureldinYosri marked this pull request as ready for review March 26, 2024 18:49

NoureldinYosri requested review from wcourtney, vtomole, cduck, verult and a team as code owners March 26, 2024 18:49

eliottrosenberg approved these changes Mar 27, 2024

View reviewed changes

NoureldinYosri and others added 2 commits March 27, 2024 13:41

Add more tests

ed4238e

Merge branch 'main' into gauge_compiling

f1c68db

NoureldinYosri requested a review from pavoljuhas March 27, 2024 20:42

NoureldinYosri added 4 commits March 27, 2024 14:53

fix typo

a444e80

nit

cd157c8

order imports

7d6ebe2

Add gauge transformation for ISWAP and SQRT_ISWAP

519a1d7

NoureldinYosri requested a review from dstrain115 March 29, 2024 19:22

NoureldinYosri added 2 commits March 29, 2024 13:20

remove unnecessary type

90da760

fix typo

c90be7c

eliottrosenberg approved these changes Mar 29, 2024

View reviewed changes

cirq-core/cirq/transformers/gauge_compiling/iswap_gauge.py Outdated Show resolved Hide resolved

cirq-core/cirq/transformers/gauge_compiling/sqrt_iswap_gauge.py Outdated Show resolved Hide resolved

NoureldinYosri and others added 4 commits March 29, 2024 15:26

Address comments

d8972e0

Merge branch 'main' into gauge_compiling

53483c1

fix SQRT_ISWAP gauges

1619265

fix typo

fbd4d00

eliottrosenberg reviewed Mar 29, 2024

View reviewed changes

address comment

607d523

pavoljuhas reviewed Mar 29, 2024

View reviewed changes

pavoljuhas reviewed Mar 30, 2024

View reviewed changes

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py Outdated Show resolved Hide resolved

pavoljuhas reviewed Mar 30, 2024

View reviewed changes

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling_test.py Outdated Show resolved Hide resolved

pavoljuhas reviewed Mar 30, 2024

View reviewed changes

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling_test_utils.py Outdated Show resolved Hide resolved

eliottrosenberg requested changes Mar 30, 2024

View reviewed changes

cirq-core/cirq/transformers/gauge_compiling/iswap_gauge.py Outdated Show resolved Hide resolved

eliottrosenberg requested changes Mar 30, 2024

View reviewed changes

cirq-core/cirq/transformers/gauge_compiling/iswap_gauge.py Outdated Show resolved Hide resolved

cirq-core/cirq/transformers/gauge_compiling/iswap_gauge.py Show resolved Hide resolved

NoureldinYosri and others added 5 commits April 1, 2024 10:51

address comments

8329ec2

nits

9d79f78

Merge branch 'main' into gauge_compiling

76d4aca

use attrs

3a22cd7

lint

82d1393

pavoljuhas requested changes Apr 1, 2024

View reviewed changes

pavoljuhas reviewed Apr 2, 2024

View reviewed changes

cirq-core/cirq/transformers/gauge_compiling/gauge_compiling.py Outdated Show resolved Hide resolved

NoureldinYosri and others added 3 commits April 2, 2024 14:20

address comments

86860cd

replace xfail with flag

ea0453b

Merge branch 'main' into gauge_compiling

35f52d6

NoureldinYosri requested review from pavoljuhas and eliottrosenberg April 2, 2024 21:33

eliottrosenberg approved these changes Apr 2, 2024

View reviewed changes

Merge branch 'main' into gauge_compiling

87ab1bb

pavoljuhas approved these changes Apr 3, 2024

View reviewed changes

nit

5dcc99f

NoureldinYosri enabled auto-merge (squash) April 3, 2024 00:43

Merge branch 'main' into gauge_compiling

db24b44

NoureldinYosri merged commit f9f0d66 into quantumlib:main Apr 3, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce gauge compilation #6526

Introduce gauge compilation #6526

NoureldinYosri commented Mar 26, 2024 •

edited

Loading

codecov bot commented Mar 26, 2024 •

edited

Loading

eliottrosenberg left a comment

eliottrosenberg left a comment

NoureldinYosri commented Mar 29, 2024

pavoljuhas Mar 29, 2024

NoureldinYosri Mar 30, 2024

pavoljuhas Apr 1, 2024

NoureldinYosri Apr 1, 2024

pavoljuhas Mar 30, 2024

NoureldinYosri Apr 1, 2024

pavoljuhas Mar 30, 2024

NoureldinYosri Apr 1, 2024 •

edited

Loading

pavoljuhas left a comment

pavoljuhas Apr 1, 2024

NoureldinYosri Apr 1, 2024

pavoljuhas Apr 1, 2024

NoureldinYosri Apr 1, 2024

pavoljuhas Apr 1, 2024

NoureldinYosri Apr 2, 2024

pavoljuhas Apr 1, 2024

NoureldinYosri Apr 2, 2024

eliottrosenberg left a comment

pavoljuhas left a comment

pavoljuhas Apr 3, 2024

pavoljuhas Apr 3, 2024

NoureldinYosri Apr 3, 2024



		def _select(choices: Sequence[Gauge], probabilites: np.ndarray, prng: np.random.Generator) -> Gauge:
		return choices[prng.choice(len(choices), p=probabilites)]

Introduce gauge compilation #6526

Introduce gauge compilation #6526

Conversation

NoureldinYosri commented Mar 26, 2024 • edited Loading

codecov bot commented Mar 26, 2024 • edited Loading

Codecov Report

eliottrosenberg left a comment

Choose a reason for hiding this comment

eliottrosenberg left a comment

Choose a reason for hiding this comment

NoureldinYosri commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoureldinYosri Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

pavoljuhas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliottrosenberg left a comment

Choose a reason for hiding this comment

pavoljuhas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoureldinYosri commented Mar 26, 2024 •

edited

Loading

codecov bot commented Mar 26, 2024 •

edited

Loading

NoureldinYosri Apr 1, 2024 •

edited

Loading