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.

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

Merge serializable_forms and deserialized_forms #6520

Merged

Conversation

verult
Copy link
Collaborator

@verult verult commented Mar 22, 2024

This PR simplifies the experience of updating the supported gateset for GridDevice.

The original reasons of separating of serializable_forms and deserialized_forms were twofolds:

  1. FSimGateFamily(gates_to_accept=[cirq.CZ]) does not accept cirq.GateFamily(cirq.CZ), and similarly for other 2q gates.
  2. The deserialized gateset should be as simple as possible to accept all valid gates, i.e.:
  • It should contain the minimum set of gates / GateFamilies to accept all valid gates (for example FSimGateFamily(gates_to_accept=[cirq.CZ]) and cirq.GateFamily(cirq.CZ) were considered redundant for accepting the CZ gate).
  • When possible, prefer gate types over GateFamilies (e.g. cirq.PhasedXZGate instead of cirq.GateFamily(cirq.PhasedXZGate)).

I prefer to merge these fields now because

  1. Having a single field is much easier for developers who wants to update the gateset to understand.
  2. The original design decision was that cirq.GateFamily(cirq.CZ) shouldn't be accepted by the gateset. Although rare, I don't think this is what we want.
  3. Adding GateFamilies instead of gate types is not much more complicated, and is valuable if it simplifies the experience of updating the gateset.

cc @wcourtney

@verult verult requested review from wcourtney, vtomole, cduck and a team as code owners March 22, 2024 18:38
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.78%. Comparing base (decf16d) to head (d81a17d).
Report is 2 commits behind head on main.

❗ Current head d81a17d differs from pull request most recent head d037f0f. Consider uploading reports for the commit d037f0f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6520      +/-   ##
==========================================
- Coverage   97.78%   97.78%   -0.01%     
==========================================
  Files        1105     1105              
  Lines       95109    95109              
==========================================
- Hits        93000    92999       -1     
- Misses       2109     2110       +1     

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

Copy link
Collaborator

@dstrain115 dstrain115 left a comment

Choose a reason for hiding this comment

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

This seems fine to me.

@verult verult enabled auto-merge (squash) March 22, 2024 20:40
@verult verult merged commit edd8393 into quantumlib:main Mar 22, 2024
29 checks passed
jselig-rigetti pushed a commit to jselig-rigetti/Cirq that referenced this pull request May 28, 2024
* Merge serializable_forms and deserialized_forms

* Documentation for adding gates

* Update documentation for adding CompilationTargetGatesets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants