-
-
Notifications
You must be signed in to change notification settings - Fork 21
🔧 Replace mypy with ty
#572
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
📝 WalkthroughWalkthroughMigrate static typing from mypy to Ty: enable Ty in CI, replace pre-commit mypy hook with a local Changes
Sequence Diagram(s)(Skipped — changes are tooling migration and scattered typing/signature updates, not a new multi-component runtime control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/mqt/predictor/ml/helper.py`:
- Around line 121-123: The call site uses qc.count_ops() (returns a
Counter/Mapping-like object) and currently silences the type mismatch with a
ty-ignore; update the signature of dict_to_featurevector to accept a
Mapping[str, int] (or collections.abc.Mapping) instead of a concrete dict so it
directly accepts ops_list without casting, remove the ty: ignore in the call,
and add any needed typing import (Mapping) and adjust internal code in
dict_to_featurevector to treat the input as a mapping of str->int.
In `@src/mqt/predictor/rl/predictorenv.py`:
- Around line 17-18: Remove the unused noqa marker on the import of TketBasePass
in predictorenv.py: delete the trailing "# noqa: PLC2701" from the line
importing pytket._tket.passes.BasePass (aliased as TketBasePass) so the import
is a plain statement alongside the QiskitBasePass import; ensure no other noqa
markers are added for TketBasePass or QiskitBasePass.
In `@tests/compilation/test_integration_further_SDKs.py`:
- Line 42: The parameter annotation for available_actions_dict is wrong: replace
dict[str, list[Action]] with dict[PassType, list[Action]] wherever used (e.g.,
in test_bqskit_o2_action and the other test functions that accept
available_actions_dict) so the type matches the fixture returning keys of type
PassType; update the function signatures (and any related type hints) to use
PassType as the dict key type and ensure PassType is imported where needed.
In `@tests/hellinger_distance/test_estimated_hellinger_distance.py`:
- Around line 254-258: The test uses rng.integers(0, 10) which can produce 0 and
generates 1‑D feature arrays; change to a deterministic positive sample count
(e.g. set random_int = 5) and construct X_train as a 2‑D array (e.g.
rng.random((random_int, n_features))) while keeping y_train as a 1‑D array of
length random_int so TrainingData(X_train=..., y_train=...) provides a non-empty
2‑D feature matrix compatible with RandomForestRegressor and avoids spurious
failures when checking device count.
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/mqt/predictor/rl/predictorenv.py`:
- Around line 19-20: Remove the no-op conditional that checks "if
sys.version_info >= (3, 11) and TYPE_CHECKING: # pragma: no cover" in
predictorenv.py; delete that entire empty block (the conditional and its "pass")
so there is no dead code left behind, keeping imports and other logic intact.
In `@tests/compilation/test_integration_further_SDKs.py`:
- Line 20: The import line "from pytket._tket.passes import BasePass as
TketBasePass" contains an unnecessary "# noqa: PLC2701" directive; remove that
trailing noqa comment so the import is clean and linter warnings about unused
noqa are resolved—edit the import statement that defines TketBasePass to drop
the "# noqa: PLC2701" suffix.
♻️ Duplicate comments (1)
src/mqt/predictor/rl/predictorenv.py (1)
17-17: Remove unusednoqadirective.Static analysis flags
# noqa: PLC2701as unused on this import.🧹 Proposed fix
-from pytket._tket.passes import BasePass as TketBasePass # noqa: PLC2701 +from pytket._tket.passes import BasePass as TketBasePass
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/mqt/predictor/ml/predictor.py`:
- Line 179: The call to QuantumCircuit.from_qasm_file(filename) uses a
type-check suppression; instead normalize the argument by converting filename to
a string (e.g., use str(filename) or filename.as_posix()) before passing it and
remove the "# ty: ignore" comment; update the call site where
QuantumCircuit.from_qasm_file is invoked so it passes str(filename) (or
filename.as_posix()) to satisfy the type checker and keep the call readable.
♻️ Duplicate comments (2)
src/mqt/predictor/ml/predictor.py (2)
356-356: Same type-ignore concern as above.
Consider converting thePathtostrhere as well to avoid suppression.
466-468: Same type-ignore concern as above.
Converting thePathtostravoids the ignore and keeps the call type-safe.
burgholzer
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.
This looks reasonable 👍🏻 thanks 🙏
Description
This PR replaces
mypywithty.Checklist:
I have added appropriate tests that cover the new/changed functionality.I have updated the documentation to reflect these changes.I have added migration instructions to the upgrade guide (if needed).