Add cirq_google.FSimGateFamily class.

[tanujkhattar]

This PR adds a new cirq_google.FSimGateFamily class which can be used to:

1. Convert any compatible FSimGate type to another compatible FSimGate type.
2. Accept (potentially parameterized) instances of compatible FSimGate types.

The gate family is also used to replace existing logic in cirq_google for checking / converting against fsim types. Once merged, this would also be useful to perform similar checks and conversions in pyle.

This is part of the roadmap item: #3243

[MichaelBroughton]

as proposed in #4585. Based on the discussion on the other issue, I will pull out this independent change into a separate PR if needed.

Could we separate these two out, before we start in on the review ?

[tanujkhattar]

@MichaelBroughton The major contribution of the this PR is in the new cirq_google/ops/common_gate_families.py and it's test file.
I think we can proceed with it's review and in parallel I'll also pull out the unrelated change into the separate PR -- since this is a large PR, I don't think it's worth blocking it's review. LMK your thoughts.

[tanujkhattar]

@MichaelBroughton I removed the unrelated change and have added a manual override for the specific case. This should work for the next couple of releases -- till the other issue is fixed and has gone through a deprecation cycle before the changes are released.

This PR is ready for review.

[tanujkhattar]

@MichaelBroughton @dstrain115 Gentle reminder. The PR is ready for review.

[MichaelBroughton]

This is looking good. I've left a few comments in the implementation side of things. @dstrain115 if you want to weigh in on serializer stuff feel free.

[tanujkhattar]

Resolved all comments. The PR is ready for a re-review. @dstrain115 @MichaelBroughton

[maffoo]

nit: use isinstance (this should also allow removing the type: ignore on the next line)

Suggested change

	if type(g) == cirq.PhasedISwapPowGate:
	if isinstance(g, cirq.PhasedISwapPowGate):

[tanujkhattar]

The reason to use type equality comparison instead of isinstance was to ensure that we override this special case only for the class defined in Cirq and do not override the behavior for any user-defined derived class. So, a CustomPhasedISwapGate that derives from PhasedISwapGate and overrides the _value_equality_values_ method wouldn't be special cased here.

[maffoo]

nit: as commented above, use isinstance.

[tanujkhattar]

same here as replied above. Will resolve the comment once the discussion above is resolved.

[maffoo]

I don't quite understand why this should include types as well as instances, when gate_types_to_check already contains gate types. Can you clarify?

[tanujkhattar]

• gate_types_to_check puts an initial filter on which compatible gate instances should be checked for containment.
• gates_to_accept defines the class of compatible gate unitaries that should be accepted.

So, for example, if I want to accept all iswappow gates which can be specified as either an fsim gate or an iswap gate, I would use:

>>> cg.FSimGateFamily(
    gates_to_accept=[cirq.ISwapPowGate], 
    gate_types_to_check=[cirq.FSimGate, cirq.ISwapPowGate]
)
>>> assert cirq.PhasedISwapPowGate(phase_exponent = 0, exponent = 0.5) not in gf
>>> assert cirq.ISWAP ** 0.5 in gf

[maffoo]

I wonder if we can fix the typing issue here, by using a type var, something like:

from typing import TypeVar
...
T = TypeVar('T', bound=POSSIBLE_FSIM_GATES)
...
def convert(self, gate: cirq.Gate, target_gate_type: Type[T]) -> Optional[T]:
    ...

If this can work I think it would eliminate a bunch of casts when calling convert, but I'm not quite sure whether bounds on type vars work on Unions like this.

If the generic convert function can't be made typesafe, something we might want to do is make the specific conversion functions public so they can be called in a type-safe way, e.g. if you know you want an FSimGate you could call family.convert_to_fsim(gate) instead of cast(cirq.FSimGate, family.convert(gate, cirq.FSimGate)).

[tanujkhattar]

After getting stuck in the mypy hell for quite some time, I've made a change which seems to work and is typesafe at the callsite. Though there's an (unavoidable?) issue that if we call convert(g, target) where target's type is Union[Type[FSimGate], Type[ISwapPowGate], etc.] (for example by iterating over a non-homogenous list of conversion targets), then mypy complaints.

LMK if you have a better solution. I personally do prefer having a single convert method over having explicit methods for each target type.

[tanujkhattar]

@maffoo @dstrain115 @MichaelBroughton Gentle reminder. The PR is waiting for a re-review. Thanks!

[dstrain115]

Only minor nits left in my review.

[dstrain115]

optional nit: can we combine these two docstrings?

[tanujkhattar]

Since the two value errors get triggered in different scenarios, I'd prefer to keep them separate.

[dstrain115]

Should this be a module-level constant instead of a member variable instead?

[tanujkhattar]

This should be a member variable because it's a dictionary where values are member functions of the class. The _convert_to_* need to be member functions instead of free functions because they in turn call _approx_eq_or_symbol member function which depends on self.allow_symbols variable.

We could refactor to make allow_symbols a function parameter and then move the member functions out of the class but I don't see much value in doing that. LMK if you feel otherwise.

[tanujkhattar]

@dstrain115 Thanks for the review. I've addressed your comments.

I'll merge once @maffoo takes another look.

[maffoo]

Minor comments, then LGTM.

[maffoo]

Seems odd here to convert to dict but then back to tuple which iterates over the dict keys and ignores the value anyway. I'd suggest converting to set instead:

Suggested change

	self.gates_to_accept = tuple(dict.fromkeys(gates_to_accept))
	self.gate_types_to_check = tuple(dict.fromkeys(gate_types_to_check))
	self.gates_to_accept = tuple(set(gates_to_accept))
	self.gate_types_to_check = tuple(set(gate_types_to_check))

Or just use frozenset directly instead of a tuple:

Suggested change

	self.gates_to_accept = tuple(dict.fromkeys(gates_to_accept))
	self.gate_types_to_check = tuple(dict.fromkeys(gate_types_to_check))
	self.gates_to_accept = frozenset(gates_to_accept)
	self.gate_types_to_check = frozenset(gate_types_to_check)

[tanujkhattar]

The goal here is to keep only the unique values from the input but also preserve the input ordering so that repr/str look as expected to the users. Iterable --> dict --> tuple is a succinct way to achieve this.

This has also been done earlier for gatesets, which had a similar requirement:

Cirq/cirq-core/cirq/ops/gateset.py

Line 215 in 5428606

unique_gate_list: List[GateFamily] = list(

Stale review, merging now.