Use cirq.q instead of str in analog #7736
Conversation
BichengYing
requested review from
a team,
hoisinberg,
verult,
vtomole and
wcourtney
as code owners
cirq-google/cirq_google/experimental/analog_experiments/analog_trajectory_util.py
Outdated
Show resolved
Hide resolved
cirq-google/cirq_google/experimental/analog_experiments/generic_analog_circuit_test.py
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7736 +/- ##
==========================================
- Coverage 99.38% 99.37% -0.01%
==========================================
Files 1091 1091
Lines 97919 97935 +16
==========================================
+ Hits 97313 97327 +14
- Misses 606 608 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size: L
| def _coupler_name(coupler: cgc.Coupler) -> str: | ||
| q1, q2 = sorted(coupler.qubits) | ||
| return f"c_q{q1.row}_{q1.col}_q{q2.row}_{q2.col}" # type: ignore[attr-defined] |
There was a problem hiding this comment.
Do we need this function?
There was a problem hiding this comment.
Yeah, that is because analog code is still quite close to the internal code. The str coupler representation is still needed in the low-level gates implementation. (We don't wanna to introduce the circular import issue there)
| gac._to_grid_qubit("q1") | ||
|
|
||
|
|
||
| def test_coupler_name_from_qubit_pair() -> None: |
There was a problem hiding this comment.
I think the name of this should change. Also, we may not need the function that it is testing.
| Args: | ||
| sparse_trajectory: A list of tuples, where each tuple defines a `FrequencyMap` | ||
| and contains three elements: (duration, qubit_freqs, coupling_strengths). | ||
| `duration` is a tunits value, `qubit_freqs` is a dictionary mapping qubit strings |
There was a problem hiding this comment.
Please fix doc string ("qubit strings")
| qubits: list[str], | ||
| pairs: list[tuple[str, str]], | ||
| qubits: list[cirq.Qid], | ||
| pairs: list[cgc.Coupler], |
There was a problem hiding this comment.
Maybe call it couplers instead of pairs?
There was a problem hiding this comment.
Yeah, modified.
| to detuning frequencies, and `coupling_strengths` is a dictionary mapping qubit | ||
| pairs to their coupling strength. This format is considered "sparse" because each | ||
| to detuning frequencies, and `coupling_strengths` is a dictionary mapping | ||
| coupler to their coupling strength. This format is considered "sparse" because each |
There was a problem hiding this comment.
Grammar: "coupler" --> "couplers"
| Args: | ||
| sparse_trajectory: A list of tuples, where each tuple defines a `FrequencyMap` | ||
| and contains three elements: (duration, qubit_freqs, coupling_strengths). | ||
| `duration` is a tunits value, `qubit_freqs` is a dictionary mapping qubit strings |
There was a problem hiding this comment.
Should we also accept cirq.Duration? If not, should we deprecate cirq.Duration?
There was a problem hiding this comment.
We yse tu.Value here is requested by Trond because "duration" here is parameterized commonly. Resolving it into cirq.Duration is awkward but to t-units: it is simply cirq.Point("t_r", [1,2,3,4,5]*tu.ns)
dstrain115
left a comment
dstrain115
left a comment
There was a problem hiding this comment.
Backwards incompatible change, but no one is using this functionality yet, so it's fine. Better to get this correct before usage begins.
3 participants
We should use the internal str style representation, it is natural to use cirq.q and cirq.Coupler in the analog construction.