quantumlib / Cirq

A Python framework for creating, editing, and invoking Noisy Intermediate Scale Quantum (NISQ) circuits.
Apache License 2.0
4.24k stars 1.01k forks source link

Added `MSGate` to top level #6466

Closed prakharb10 closed 6 months ago

prakharb10 commented 7 months ago

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?~

pavoljuhas commented 7 months ago

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

codecov[bot] commented 7 months ago

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 ```diff @@ 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.

pavoljuhas commented 7 months ago

@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.

tanujkhattar commented 7 months ago

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

pavoljuhas commented 7 months ago

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 commented 6 months ago

@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.

prakharb10 commented 6 months ago

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.

pavoljuhas commented 6 months ago

@tanujkhattar - can you please take a quick look?