Internally we also have dataclasses to store just the parameters in these different representations with functions to canonicalize them (since the various phase parameters are periodic) and to convert between them. It might be nice to include that code here, though it might be awkward to support symbolic values so would have to try it out. Note that internally we call the canonical parametrization above FsimPhase while this alternate parametrization with per-qubit z rotations is called FsimRz. If we stick with that nomenclature then the factory function would better be called PhasedFSimGate.from_fsim_rz or something like that, and the properties for getting the rotation angles before and after might also best be renamed as rz_angles_before and rz_angles_after.

+1, it would be nice to follow up on this because it will be one of the building blocks of the some calibration features.

Renamed the factory method and the two properties as suggested. As discussed on a different PR, I'd like to defer dataclasses (and possibly re-use existing code that @mrwojtek is introducing elsewhere).

I'd suggest writing this in terms of rotations, and can also use names that correspond with the docstring:

b0, b1 = self.phase_angles_before
a0, a1 = self.phase_angles_after
yield cirq.rz(q0, b0)
yield cirq.rz(q1, b1)
yield FSimGate(self.theta, self.phi).on(q0, q1)
yield cirq.rz(q0, a0)
yield cirq.rz(a1, a1)

This is nearly identical to my original implementation. Unfortunately, protocol consistency checks are sensitive to global phase, so we have to give up __unitary__()[0, 0] == 1 or we have to yield cirq.Z instead of cirq.rz here. I chose the latter so that __unitary__ agrees with eq (43) from QS supplement.

(Per other comments, I'll be updating this PR to agree with eq (18) in FH paper, but this merely renames existing "delta" parameters, so I think I'll keep the constraint that __unitary__()[0, 0] == 1.)

(We could also yield a global phase gate, but that seems even uglier.)

I see. In that case, perhaps include a comment why this differs from the docstring, which shows Rz gates. In addition, would be good to refactor to avoid evaluating each angle property twice, and you might also consider following the rz implementation which uses sympy.pi where appropriate to preserve symbolic values.

Good suggestions. All done.

I just started to wonder about this: what does it mean that this gate implements InterchangeableQubitsGate? Does it mean that operation is equal after qubits are swapped? That's not true, right?

Thanks for catching this! Sent #3524.