Cirq icon indicating copy to clipboard operation
Cirq copied to clipboard

Added `MSGate` to top level

Open prakharb10 opened this issue 1 year ago • 7 comments

Closes #6354

~The JSON serializability test fails due to cirq_ionq.ionq_native_gates.MSGate. I don't know how to resolve this.~


~There is also a reference to the gate here:~ https://github.com/quantumlib/Cirq/blob/6e38a2741a0710542baa202ce57acb896eb469a7/cirq-core/cirq/ops/gate_operation_test.py#L511

~Are any changes required?~

prakharb10 avatar Feb 15 '24 04:02 prakharb10

cirq csynque meeting - there is a name conflict with ionq gate which shows in json serialization. recommendation - mark cirq.MSGate as not-yet-seriazable.

pavoljuhas avatar Feb 28 '24 18:02 pavoljuhas

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.76%. Comparing base (0f4822b) to head (f959478).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6466      +/-   ##
==========================================
- Coverage   97.76%   97.76%   -0.01%     
==========================================
  Files        1105     1105              
  Lines       95030    95028       -2     
==========================================
- Hits        92902    92900       -2     
  Misses       2128     2128              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 29 '24 01:02 codecov[bot]

@tanujkhattar - should we support JSON serialization now? If so, that should be doable by adding _json_namespace_ class method so we can distinguish between cirq.MSGate and cirq_ionq.MSGate.

If we don't want serialization we should make cirq.to_json(cirq.ms(1)) fail with ValueError in a similar way as LinearDict here so we don't produce unusable JSON.

On a second look, adding serialization support seems better, because it is a bit strange to support MSGate in the cirq-ionq extension, but not in cirq-core.

pavoljuhas avatar Feb 29 '24 01:02 pavoljuhas

It'll probably be backwards incompatible, which is a bummer. But we don't have backwards compatibility guarantees for vendor packages so we can break users if we want

tanujkhattar avatar Feb 29 '24 01:02 tanujkhattar

It'll probably be backwards incompatible, which is a bummer. But we don't have backwards compatibility guarantees for vendor packages so we can break users if we want

As it is, the round trip cirq.read_json(json_text=cirq.to_json(cirq.ms(1))) does not work in the main branch. It either fails with ValueError because "MSGate" type is unknown, or - after importing cirq_ionq - with TypeError, because cirq_ionq.MSGate has different arguments.

What I suggest is to define cirq.MSGate._json_namespace_ returning "cirq". That will distinguish the 2 gate types and would be backwards compatible, because cirq.MSGate serialization does not work anyway.

pavoljuhas avatar Feb 29 '24 02:02 pavoljuhas

@prakharb10 - are you available to add serialization support for the cirq.MSGate here?

Feel free to let me know if you are too busy and I can take over from here.

pavoljuhas avatar Mar 05 '24 22:03 pavoljuhas

Hi @pavoljuhas I tried adding serialization, but some other tests were failing. I was busy, but I will push the commit with the changes by tomorrow so that you can look at the failing tests.

prakharb10 avatar Mar 06 '24 17:03 prakharb10

@tanujkhattar - can you please take a quick look?

pavoljuhas avatar Mar 16 '24 01:03 pavoljuhas