cirq-core target gatesets: accept additional gates to keep untouched.

[verult]

Builds on top of #5429

The internal gate representation for additional_gates is updated to match cirq.Gateset:

• Equality check uses GateFamily representation. Otherwise different representations of the gate will not be considered equal.
• JSON uses GateFamily representation.
• repr uses the representation passed in via the constructor.

assert_optimizes in cz_gateset_test.py is updated to take in an optional additional_gates instead, to exercise CZTargetGateset constructor's defaulting logic.

No tests are added since additional_gates need to be set in existing tests after ignore_errors is set to False.

@tanujkhattar

[tanujkhattar]

Looking good overall. Main feedback is around changing serialization logic to avoid modifying all existing usage sites.

[tanujkhattar]

also include a gate instance and gate family instance in the additional_gates list.

[verult]

I'll also add extra gates in SqrtIswapTargetGateset.repr and CZTargetGateset.repr.

Do repr tests such as this one cover more than what json serialization tests cover?

[tanujkhattar]

The json tests check that eval(rep_text_from_file) is equal to cirq.read_json(json_text=json_from_file) whereas the repr tests check that eval(repr(x)) == x.

So, it's possible that repr(x) for an object x doesn't produce a valid text output for which eval(repr(x)) evaluates to the same object. But, for the same object, the developers can make a mistake and copy the correct text in .repr file which does evaluate to the expected object.

I've seen the latter happen in the past, where the text in x.repr files didn't actually match the text produced by calling repr(x) on the object, which is bad but is not verified by the json tests.

[verult]

Ack. I can add a repr test to CZ gateset as well. In the long term it might be useful to have a test that verifies repr(eval(x.repr)) == x.repr. I can open an after-1.0 issue to track this

[tanujkhattar]

To avoid changing the default serialization, and thereby going and changing multiple json / repr files like cg.QuantumExecutable etc, I think we can do the following:

1. Make the default additional_gates to be an empty sequence.
2. Don't print the additional_gates parameter in repr if the value is an empty sequence.

For GlobalPhaseGate, I'm considering we should include it by default similar to PhasedXZ and measurement gates. Since presence of a GlobalPhaseGate doesn't impact the circuit unitary that should be run on the device, if vendor backends don't support the global phase gate, they can always ignore it during serialization.

[verult]

Another option is to leave out GlobalPhaseGate altogether and add in the docstring that a user most likely will want to include this. One way to think about the core gate set (PhasedXZ etc) is the set of gates the target gateset can compile to. If that's the abtraction, then GlobalPhaseGate doesn't belong there. But I don't have a strong preference among these options :) including the option of keeping it as is.

[tanujkhattar]

The reason I wanted to include GlobalPhaseGate by default, instead of excluding it, is that we now preserve the global phase in all non-analytical decompositions, often by adding an explicit instance of cirq.GlobalPhaseGate to the decomposition (see #5420). And going forward, I imagine we'd also want to start preserving global phase in the analytical decompositions as well.

So, for example, if you try to compile a 3 qubit cirq.CCZPowGate gate using these target gatesets, we will fallback on the cirq.decompose protocol while expanding the composite gates (step-1 of preprocess transformers); the default expansion will add a GlobalPhaseGate to the decomposition. If the target gateset doesn't accept a global phase gate, it will raise an error. More often than not, we don't want to raise an error in this case.

For example, in most cases, even if a vendor backend doesn't support serializing global phase gates, they'd actually want to accept circuits containing global phase gates and ignore the global phase during serialization, since it doesn't have any measurable impact on the overall circuit execution.

[verult]

Got it, thanks for the explanation! Will move GlobalPhaseGate into the core gateset.

[verult]

Btw the files still need to be updated to include empty lists for additional_gates because the JSON serialization test verifies whether cirq.to_json(x) matches the content in the .json file, similar to what happened here

[verult]

Thanks for the review, @tanujkhattar . I left a few questions and also pushed the latest changes.

[verult]

@95-martin-orion would you be able to take a look at the serialization changes and approve in place of Tanuj? Serialization is the only major outstanding concern.

[verult]

I was only able to reproduce the test failure locally after pulling the latest changes from master.

[95-martin-orion]

Serialization LGTM. No idea what's causing the test failure.

[verult]

I changed the test string for now to unblock this PR. The risk is likely low since it's a string representation change. Opened issue to track it: #5546

[MichaelBroughton]

I think this won't merge due to @tanujkhattar 's still remainding changes requested here:

All concerns have been addressed - dismissing review to unblock merging.