Gate tagged with `cirq.google.PhysicalZTag` does not serialize

[jarthurgross]

Description of the issue

Cannot serialize a gate with the cirq.google.PhysicalZTag

How to reproduce the issue

>>> import cirq
>>> q = cirq.LineQubit(0)
>>> physical_gate = cirq.Z(q).with_tags(cirq.google.PhysicalZTag)
>>> physical_gate
cirq.TaggedOperation(cirq.Z(cirq.LineQubit(0)), <class 'cirq.google.ops.physical_z_tag.PhysicalZTag'>)
>>> cirq.to_json(physical_gate)
Traceback (most recent call last):
...
TypeError: _json_dict_() missing 1 required positional argument: 'self'

Cirq version

0.10.0

[maffoo]

You need to use an instance of PhysicalZTag, rather than the class itself:

>>> import cirq
>>> q = cirq.LineQubit(0)
>>> physical_gate = cirq.Z(q).with_tags(cirq.google.PhysicalZTag())
>>> physical_gate
cirq.TaggedOperation(cirq.Z(cirq.LineQubit(0)), cirq.google.PhysicalZTag())
>>> print(cirq.to_json(physical_gate))
{
  "cirq_type": "TaggedOperation",
  "sub_operation": {
    "cirq_type": "SingleQubitPauliStringGateOperation",
    "pauli": {
      "cirq_type": "_PauliZ",
      "exponent": 1.0,
      "global_shift": 0.0
    },
    "qubit": {
      "cirq_type": "LineQubit",
      "x": 0
    }
  },
  "tags": [
    {
      "cirq_type": "PhysicalZTag"
    }
  ]
}

[maffoo]

Admittedly, this is a bit annoying for tags like PhysicalZTag that are clearly singletons, so we should see if there's a way to clean that up.

[jarthurgross]

I see; thanks for pointing out my mistake. I suppose this also means that all the gates I thought were actually physical Z gates are actually not...

[viathor]

I think it would make sense for with_tags to check the type and raise an exception with informative error message to avoid this in future.

[maffoo]

@viathor, currently with_tags doesn't do any checking about what kind of objects are used as tags. I seem to recall that we discussed requiring all tags to inherit from some marker trait (a hypothetical cirq.Tag) but decided against this to avoid over-constraining the tag semantics (e.g., there may be cases where you want to use str or some other type you don't control as a tag, and can't change it to inherit from a marker trait). We may want to revisit that decision.

Another possibility in this particular case would be to change our serialization code to check for the PhysicalZTag class or instances of that class and treat them the same way to avoid this particular pitfall. Or, we could change the class to _PhysicalZTag and make PhysicalZTag be a singleton instance of it (with a __call__ method that returns itself for compatibility). It has always irked me that we have to create instances when this is clearly a singleton, and it's clear from this issue that this is confusing for others as well.

[viathor]

We could also special-case the type type:

if isinstance(tag, type):
    tag = tag()

(This will certainly upset mypy. There is a longer better way of writing this. This snippet is just to get the idea across.)

Downside here is that if someone passes a tag class whose ctor requires arguments the error message may be even more confusing...

[balopat]

My input is that a Union[str,cirq.Tag] type signature would be a good direction. Allows for str tags as well as any instance of cirq.Tag. Also +1 for adding singletons for cirq.google.PhysicalZTag and a privagte _PhysicalZTag class - this would help in syntax.

[95-martin-orion]

Useful discussion on "why not use strings for tags?" can be found in #2407.

[Dripto]

just wanted to +1 @viathor's suggestion for an exception to be raised if you don't use an instance. This has also come up for me with NoSyncTag() in the past.