-
-
Notifications
You must be signed in to change notification settings - Fork 37
🔧 Replace mypy with ty
#770
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
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces mypy with the "ty" type checker across CI, pre-commit, and pyproject; converts several simulator backend _run_experiment methods to instance methods; tightens some backend/provider type annotations; updates qasm backend run signature to accept run_input and options-based parameter_values; converts tests to pass parameter mappings as dicts; deletes a Grover test script. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/python/primitives/test_sampler.py (1)
173-179: Preferstrict=Truehere to catch parameter-length mismatches.
strict=Falseweakens test validation; if parameter counts are known,strict=Trueis safer. If you must support Python <3.10, dropstrictand add explicit length checks instead.♻️ Proposed change
- parameters_1_a = dict(zip(qc_1.parameters, [0, 1, 1, 2, 3, 5], strict=False)) - parameters_1_b = dict(zip(qc_1.parameters, [1, 2, 3, 4, 5, 6], strict=False)) - parameters_2 = dict(zip(qc_2.parameters, [0, 1, 2, 3, 4, 5, 6, 7], strict=False)) + parameters_1_a = dict(zip(qc_1.parameters, [0, 1, 1, 2, 3, 5], strict=True)) + parameters_1_b = dict(zip(qc_1.parameters, [1, 2, 3, 4, 5, 6], strict=True)) + parameters_2 = dict(zip(qc_2.parameters, [0, 1, 2, 3, 4, 5, 6, 7], strict=True))
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 367: Remove the unnecessary test dependency "qiskit-algorithms>=0.4.0"
from pyproject.toml: locate the entry that exactly matches
"qiskit-algorithms>=0.4.0" (the string from the diff) and delete that line from
the test/dev dependencies section so it is no longer declared in the project's
dependency list.
In `@python/mqt/ddsim/qasm_simulator_backend.py`:
- Around line 134-159: Update the run() method docstring to reflect that
parameter_values is no longer a separate argument but an optional key inside
**options; mention that run_input accepts a QuantumCircuit or
Sequence[QuantumCircuit], that parameter_values (Sequence[Parameters] | None)
can be provided via options with that type, and that other run options may be
passed through; keep the Returns description for DDSIMJob unchanged and
reference the method name run and the parameter_values option for clarity.
♻️ Duplicate comments (1)
test/python/primitives/test_estimator.py (1)
145-148: Same Python-version constraint applies tozip(..., strict=True)here.
Please ensure the project’s minimum Python version is 3.10+ (or removestrict=and add explicit length checks for older versions).Also applies to: 198-200, 214-223, 245-277
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/mqt/ddsim/qasm_simulator_backend.py (1)
190-190: Type annotation inconsistency in_run_experimentmethod.The
**optionsparameter is annotated asdict[str, Any]but should beAnyto match the unpacking semantics. When using**kwargs, the annotation should be the value type, not a dict type.🔧 Proposed fix
- def _run_experiment(self, qc: QuantumCircuit, **options: dict[str, Any]) -> ExperimentResult: + def _run_experiment(self, qc: QuantumCircuit, **options: Any) -> ExperimentResult:
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 `@python/mqt/ddsim/deterministic_noise_simulator_backend.py`:
- Line 52: The _run_experiment signature currently uses **options: Any and a
redundant noqa; replace that by defining a TypedDict (e.g., ExperimentOptions)
listing the accepted option keys and types, then change the signature to accept
**options: Unpack[ExperimentOptions] (importing TypedDict and Unpack from
typing/typing_extensions as needed) so ANN401 is resolved, and remove the "#
noqa: PLR6301" token from the _run_experiment definition; update any internal
references to options to match the new typed keys if necessary.
In `@python/mqt/ddsim/stochastic_noise_simulator_backend.py`:
- Line 55: Replace the loose **options: Any in _run_experiment with a concrete
TypedDict and Unpack to tighten typing: create an ExperimentOptions TypedDict
containing the exact keys and value types that _run_experiment expects, import
TypedDict and Unpack from typing, change the signature to def
_run_experiment(self, qc: QuantumCircuit, **options: Unpack[ExperimentOptions])
-> ExperimentResult, and remove the unused "# noqa: PLR6301" comment; also
update any local references or callers to match the new option key names/types
if necessary.
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.
LGTM 👍🏻
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).