-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Move Coupler to cirq_gooogle.ops #6697
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
Conversation
dstrain115
commented
Aug 1, 2024
- Noticed when adding code that there were circular imports due to Couplers api codes needing code from devices.
- Moved this to ops directory instead, which avoids the circular import.
- Note that, although GridQubit is in cirq.devices, LineQubit and NamedQubit are in cirq.ops, so this also seems to be a good place to put this class.
- Noticed when adding code that there were circular imports due to Couplers api codes needing code from devices. - Moved this to ops directory instead, which avoids the circular import. - Note that, although GridQubit is in cirq.devices, LineQubit and NamedQubit are in cirq.ops, so this also seems to be a good place to put this class.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6697 +/- ##
=======================================
Coverage 97.82% 97.82%
=======================================
Files 1074 1074
Lines 92191 92187 -4
=======================================
- Hits 90189 90186 -3
+ Misses 2002 2001 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with 2 nitpicky comments.
@@ -77,7 +72,6 @@ def qubit_from_proto_id(proto_id: str) -> cirq.Qid: | |||
A `cirq.Qid` corresponding to the proto id. | |||
""" | |||
# Avoid circular import |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT - remove outdated comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
from cirq_google.experimental import ops as experimental_ops | ||
from cirq_google.ops.coupler import Coupler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of consistency with ops.Stuff
syntax at 49-52, can we drop this import and use ops.Coupler
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added as cg_ops (it's cirq_google.ops)
@@ -38,7 +38,7 @@ | |||
from cirq_google.api import v2 | |||
from cirq_google.devices import known_devices | |||
from cirq_google.experimental import ops as experimental_ops | |||
from cirq_google.ops.coupler import Coupler | |||
from cirq_google import ops as cg_ops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's then delete line 36 and only use cg_ops
for that module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missed that this was cirq_google.ops (thought it was cirq.ops). Reverted this and changed to ops.Coupler for brevity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us use only one of cg_ops
, ops
for cirq_google.ops
.
* Move Coupler to cirq_gooogle.ops - Noticed when adding code that there were circular imports due to Couplers api codes needing code from devices. - Moved this to ops directory instead, which avoids the circular import. - Note that, although GridQubit is in cirq.devices, LineQubit and NamedQubit are in cirq.ops, so this also seems to be a good place to put this class.