-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add return-type to public functions, mostly tests part 5 #7691
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
Add return-type to public functions, mostly tests part 5 #7691
Conversation
Let mypy type check accept `cirq.Operation ** exponent`.
The value is converted to tuple in `protocols.kraus()` anyway. This makes PhaseDampingChannel match the SupportsKraus type.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7691 +/- ##
==========================================
- Coverage 99.38% 99.38% -0.01%
==========================================
Files 1089 1089
Lines 97526 97539 +13
==========================================
+ Hits 96925 96937 +12
- Misses 601 602 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mhucka
left a 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.
Looks good. My only comment/question is whether it would be worth adding in the PR description that this makes changes to a few type declarations, to fix bugs (if indeed they are bug fixes).
| qubits: Sequence[cirq.Qid] | None = None, | ||
| atol: float = 1e-8, | ||
| ) -> cirq.OP_TREE: | ||
| ) -> Sequence[cirq.Operation]: |
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.
At first I thought this could count as an API change, but reading the function, it looks like it has always returned a sequence. So is this a bug fix to the type definition, then? If so, is it perhaps worth mention that in the PR description? (I.e., that in some cases, that some of the changes include fixes.)
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.
It is a fix in type annotation to make it consistent with the actual value returned. The original annotation would fail type-check because the output is used elsewhere as a for-loop iterable, but cirq.OP_TREE may be a scalar cirq.Operation too.
The change is safe, because Sequence[Operation] satisfies the OP_TREE type, albeit with a more narrow scope. In other words, any caller that expected OP_TREE from decompose_cphase_into_two_fsim still gets an OP_TREE-compatible object.
In Python type annotations are thus far only used for static type check and are completely ignored at runtime. I don't quite see a benefit of putting that level of detail to PR description, otherwise this PR comments should help the interested reader. :)
| context: transformer_api.TransformerContext | None = None, | ||
| prng: np.random.Generator | None = None, | ||
| ) -> tuple[circuits.AbstractCircuit, cirq.Sweepable]: | ||
| ) -> tuple[circuits.AbstractCircuit, cirq.Sweep]: |
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.
A similar question here, about this perhaps being a bug fix (changing an existing return type declaration).
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.
The as_sweep function returns a tuple of (cirq.Circuit, cirq.Zip) where Zip is a concrete realization of the Sweep abstract class
| return circuits.Circuit.from_moments(*new_moments), Zip(*sweeps) |
The original Sweepable is a union that includes Sweep and several other types with different interfaces. The new Sweep type satisfies Sweepable, but is more useful, because the type is narrowed to one ABC (ie, object methods and APIs are known), rather than to a union of unrelated types.
This PR series for adding type checks to unit tests will always uncover some annotation issues that need to be corrected. Some of them may need mention in PR description, but it would be too painstaking and possibly confusing to list them all. I feel this change is of the latter kind.
No change in the effective code. A batch of ~50 files.
Modified files pass ruff check --select=ANN201
Notable changes:
Operation.__pow__to support static type checklist[Qid]-->Sequence[Qid]which is covariantKakDecompositionandFSimGatematch theSupportsUnitarytype_kraus_method fromSequencetoIterablePartially implements #4393