It looks like there is a significant diffbase in here for neutral atoms and the PR is marked as Draft, so I will hold off reviewing for now. Can you ping reviewers when it is ready?

@dstrain115 This is ready for review now -- I had marked it as draft after realizing the git branching issue which included diffbase from the other neutral atoms PR.

@dstrain115 Gentle reminder. This is ready for review.

Drive-by review comment: Would this changeover let people do things like ?

my_gateset = cirq.GateSet(X**0.5, ignore_global_phase=True)
cirq.X ** 0.5 in my_gateset # True
cirq.PhasedXPowGate(phase_exponent=0) in my gateset # !True!

my_gateset2 = cirq.GateSet(X**0.5, ignore_global_phase=False)
cirq.X ** 0.5 in my_gateset2 # True
cirq.PhasedXPowGate(phase_exponent=0) in my gateset2 # False

This seems a little scary if true.

In [2]: cirq.PhasedXPowGate(phase_exponent=0, exponent=0.5) == cirq.X ** 0.5
Out[2]: True

Therefore, the example that you quoted would return true in both the cases. I would also not include an explicit isinstance check when doing approximate equality because it would modify the behavior of already well known concepts in Cirq (i.e. Value Equality and Equality ignoring global phase).

Ahhhh that makes a lot of sense. In my explorations I actually used cirq.PhasedXZGate which does not pass this (I then assumed PhasedX also wouldn't):

>>> cirq.PhasedXZGate(x_exponent=0.5, z_exponent=0, axis_phase_exponent=0) == cirq.X**0.5
False

But I think that's actually a bug having done a bit more exploring lol.

This makes a lot more sense now. My only remaining concern is that I think we should default to having ignore_global_phase=False starting with the strictest requirements and then if a user wants to back it off slightly they can set ignore_global_phase=True.

The reason I've chosen the default value to be True is because that's what is being used across the codebase via the legacy checks. Eg: At all the places where cirq.Gateset was used to replace existing isinstance checks, the default value would need to be True. If we set the default to False, it would make the common use case more cumbersome, i.e.

cirq.Gateset(cirq.GateFamily(cirq.X ** 0.5, ignore_global_phase = True))

instead of

cirq.Gateset(cirq.X ** 0.5)

Also, note that accept_global_phase in the Gateset parameter, which accepts global phase operation, is also set to true by default. Ignoring global phase in GateFamily checks would keep things consistent in the sense that global phase is ignored by default when checking for acceptance, be it with individual operations differing in global phase (checked via GateFamily) or two circuits with different GlobalPhaseOperations tagged on (checked via Gateset)

Now that we are talking about it, I think we should also rename the Gateset parameter accept_global_phase to accept_global_phase_op to make the distinction between the ignore_global_phase parameter on GateFamily a bit less confusing since these two parameters control different behaviors.

I've renamed accept_global_phase to accept_global_phase_op as well. This would also have been a breaking change but since we haven't released Gatesets yet, we don't need to go through a deprecation cycle.

@MichaelBroughton Thanks for the approval. I've addressed the nits. Merging now.

Automerge cancelled: A required status check is not present.

Missing statuses: ['Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage']