Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added MSGate to top level #6466

Merged
merged 9 commits into from
Mar 16, 2024
Merged

Added MSGate to top level #6466

merged 9 commits into from
Mar 16, 2024

Conversation

prakharb10
Copy link
Contributor

@prakharb10 prakharb10 commented Feb 15, 2024 •
edited
Loading

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:

Cirq/cirq-core/cirq/ops/gate_operation_test.py

Line 511 in 6e38a27

cirq.ops.MSGate,

Are any changes required?

@prakharb10 prakharb10 requested review from vtomole, cduck and a team as code owners February 15, 2024 04:03
@prakharb10 prakharb10 requested a review from maffoo February 15, 2024 04:03
@CirqBot CirqBot added the size: S 10< lines changed <50 label Feb 15, 2024
@pavoljuhas pavoljuhas self-requested a review February 28, 2024 18:40
@pavoljuhas
Copy link
Collaborator

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 Codecov
Copy link

codecov bot commented Feb 29, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pavoljuhas
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
pavoljuhas previously requested changes Mar 1, 2024
View reviewed changes
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a bit of offline discussion it seems better to support JSON serialization here.
As it is we can serialize cirq_ionq.MSGate so for completeness
we should also handle the core cirq.MSGate.

The name conflict can be avoided by defining the _json_namespace_ class method for cirq.MSGate; here is an example in cirq-google.

I suggest to use the "cirq" namespace for the cirq.MSGate.

@pavoljuhas
Copy link
Collaborator

@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
Copy link
Contributor Author

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 pavoljuhas requested a review from tanujkhattar March 16, 2024 01:53
@pavoljuhas
Copy link
Collaborator

@tanujkhattar - can you please take a quick look?

@tanujkhattar tanujkhattar enabled auto-merge (squash) March 16, 2024 03:53
@tanujkhattar tanujkhattar merged commit dadfdcb into quantumlib:main Mar 16, 2024
31 checks passed
@prakharb10 prakharb10 deleted the cirq-6354 branch March 18, 2024 00:39
jselig-rigetti pushed a commit to jselig-rigetti/Cirq that referenced this pull request May 28, 2024
@prakharb10 @tanujkhattar
@pavoljuhas @jselig-rigetti
Added MSGate to top level (quantumlib#6466)
7c65f16 
* add `MSGate` to top level

* mark `MSGate` as `not_yet_serializable`

* Revert "mark `MSGate` as `not_yet_serializable`"

This reverts commit ffdb3de.

* Test json serialization of cirq.MSGate without custom resolver

Not yet passing.

* Use the "cirq" namespace to JSON-serialize `cirq.MSGate`

The namespace-less cirq_type `MSGate` is used by `cirq_ionq.MSGate`.

---------

Co-authored-by: Tanuj Khattar <tanujkhattar@google.com>
Co-authored-by: Pavol Juhas <juhas@google.com>
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
@prakharb10 @tanujkhattar @pavoljuhas
Added MSGate to top level (quantumlib#6466)
a502f68 
* add `MSGate` to top level

* mark `MSGate` as `not_yet_serializable`

* Revert "mark `MSGate` as `not_yet_serializable`"

This reverts commit ffdb3de.

* Test json serialization of cirq.MSGate without custom resolver

Not yet passing.

* Use the "cirq" namespace to JSON-serialize `cirq.MSGate`

The namespace-less cirq_type `MSGate` is used by `cirq_ionq.MSGate`.

---------

Co-authored-by: Tanuj Khattar <tanujkhattar@google.com>
Co-authored-by: Pavol Juhas <juhas@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanujkhattar tanujkhattar tanujkhattar approved these changes

@pavoljuhas pavoljuhas pavoljuhas left review comments

@vtomole vtomole Awaiting requested review from vtomole vtomole is a code owner

@cduck cduck Awaiting requested review from cduck

@maffoo maffoo Awaiting requested review from maffoo maffoo is a code owner automatically assigned from quantumlib/cirq-maintainers

Assignees
No one assigned
Labels
size: S 10< lines changed <50
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MSGate not in top level
4 participants
@prakharb10 @pavoljuhas @tanujkhattar @CirqBot