Skip to content

Create cirq_google.InternalGate #6194

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

Merged
merged 4 commits into from
Jul 17, 2023
Merged

Create cirq_google.InternalGate #6194

merged 4 commits into from
Jul 17, 2023

Conversation

NoureldinYosri
Copy link
Collaborator

This is the first step towards porting internal gates to Cirq.

@CirqBot CirqBot added the size: M 50< lines changed <250 label Jul 12, 2023
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.

LGTM with some nits.

@NoureldinYosri
Copy link
Collaborator Author

@maffoo let me know if this makes sense

@maffoo
Copy link
Contributor

maffoo commented Jul 13, 2023

Discussed over chat with @NoureldinYosri. I want to be sure we think carefully about how to handle units here, and I actually lean toward putting units in names (e.g. InternalGate("CouplerDelayZ", ..., delay_ns=1, ...)) because we've seen internally that dealing with plain numbers results in significant performance gains compared to allocating lots of objects with units (InternalGate("CouplerDelayZ", ..., delay=Value(1, "ns"), ...)). Also, we don't generally benefit much from the flexibility of doing unit conversions since most parameters for gates have one "natural" unit and the unit name is mostly for documentation. All that said, it doesn't need to block moving ahead on this PR, so LGTM.

@NoureldinYosri NoureldinYosri changed the title Create cirq_google.InternalGate Create cirq_google.InternalGate and cirq_google.InternalClass Jul 14, 2023
@NoureldinYosri
Copy link
Collaborator Author

@dstrain115 @maffoo concerning the problem of passing units it's a symptom of a larger problem which is that we won't be able to pass values that in themselves are instances of internal classes. adding a suffix to the argument name opens a can of worms for who to say that the gate doesn't have an argument with the modified name and it will be a nightmare to support and maintain. while exposing some internal classes defeats the purpose of this project.

The most suitable solution is to support internal classes the same way we support internal gates, that's why I introduced cirq_gate.InternalClass

please take another look

@NoureldinYosri
Copy link
Collaborator Author

I discarded the last change after talking with @dstrain115 since we won't support units in the first version of this work.

@NoureldinYosri NoureldinYosri changed the title Create cirq_google.InternalGate and cirq_google.InternalClass Create cirq_google.InternalGate Jul 17, 2023
@NoureldinYosri NoureldinYosri merged commit b72a532 into master Jul 17, 2023
@NoureldinYosri NoureldinYosri deleted the internal_gate branch July 19, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants