-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix mypy errors (primitives) #8263
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
I'm not a qiskit-terra maintainer, so my review does not reflect of the maintainers' preference. But in general, you can use |
The content LGTM. |
8480bc1
to
788f69d
Compare
Pull Request Test Coverage Report for Build 4490399008
💛 - Coveralls |
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.
Hi @Randl, thanks for opening this PR, I have a few general questions plus some more specific comments I have left in the code:
- I think that making sure we have correct typehints is very valuable, but given that we currently we don't run any mypy checks, I am not convinced about the value added by the constant use of
cast
(which I know, is sometimes the only way to please mypy without making significant changes in the code). My take here would be to add the typehints but not thecast
s, as they obfuscate the code and make it less readable. - I see that you fixed the merge conflicts, but you also reverted your changes on
base_sampler.py
andbase_estimator.py
, was this intentional? I think that they should be included for completeness.
@ElePT I think that some code I annotated got removed, that's why it appears that some changes are reverted |
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.
Great! I have a minor comment. I think Sequence
is too broad here.
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
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.
Thanks for applying the suggestions!!
@ElePT I think that some code I annotated got removed, that's why it appears that some changes are reverted
Sorry, I misread your comment earlier. The code did not get removed, it got moved, so those files are still part of the repo (instead of being primitives.base_estimator.py
, now they are in primitives.base.base_estimator.py
). Given that you had already looked at them, could you re-apply the changes?
I'm not sure what exactly you're reffering to. I've checked again and the remaining errors
can't be fixed without type casting as far as I understand |
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.
Never mind, I think that I was looking at an outdated version of the PR locally. LGTM.
@ikkoham can you approve please? |
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.
Thanks. LGTM
So anything else required from me before it is merged? |
@ikkoham any further changes? if not, can you approve the requested changes? |
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.
Sorry for late. Thank you for achieving my comments. LGTM!
@ElePT Thank you for asking!
* Fix primitives mypy errors * Remove casts, fix types * Apply suggestions from code review Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com> --------- Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
* Fix primitives mypy errors * Remove casts, fix types * Apply suggestions from code review Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com> --------- Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
* Fix primitives mypy errors * Remove casts, fix types * Apply suggestions from code review Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com> --------- Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Summary
Following discussion, I'm splitting #8187 by module.
Details and comments
Previous content
The are ~20 errors left: ``` qiskit/primitives/base_estimator.py:322: error: Invalid index type "Union[int, QuantumCircuit]" for "Tuple[QuantumCircuit, ...]"; expected type "SupportsIndex" [index] qiskit/primitives/base_estimator.py:342: error: Invalid index type "Union[int, QuantumCircuit]" for "Tuple[ParameterView, ...]"; expected type "SupportsIndex" [index] qiskit/primitives/base_estimator.py:345: error: Invalid index type "Union[int, QuantumCircuit]" for "Tuple[ParameterView, ...]"; expected type "SupportsIndex" [index] qiskit/primitives/base_estimator.py:349: error: Invalid index type "Union[int, QuantumCircuit]" for "Tuple[QuantumCircuit, ...]"; expected type "SupportsIndex" [index] qiskit/primitives/base_estimator.py:350: error: Invalid index type "Union[int, SparsePauliOp]" for "Tuple[SparsePauliOp, ...]"; expected type "SupportsIndex" [index] qiskit/primitives/base_estimator.py:358: error: Value of type variable "SupportsRichComparisonT" of "max" cannot be "Union[int, QuantumCircuit]" [type-var] qiskit/primitives/base_estimator.py:358: error: Unsupported operand types for <= ("int" and "QuantumCircuit") [operator] qiskit/primitives/base_estimator.py:358: note: Left operand is of type "Union[int, QuantumCircuit]" qiskit/primitives/base_estimator.py:360: error: Value of type variable "SupportsRichComparisonT" of "max" cannot be "Union[int, QuantumCircuit]" [type-var] qiskit/primitives/base_estimator.py:363: error: Value of type variable "SupportsRichComparisonT" of "max" cannot be "Union[int, SparsePauliOp]" [type-var] qiskit/primitives/base_estimator.py:363: error: Unsupported operand types for <= ("int" and "SparsePauliOp") [operator] qiskit/primitives/base_estimator.py:363: note: Left operand is of type "Union[int, SparsePauliOp]" qiskit/primitives/base_estimator.py:365: error: Value of type variable "SupportsRichComparisonT" of "max" cannot be "Union[int, SparsePauliOp]" [type-var] qiskit/primitives/base_estimator.py:370: error: Argument "circuits" to "_call" of "BaseEstimator" has incompatible type "List[Union[int, QuantumCircuit]]"; expected "Sequence[int]" [arg-type] qiskit/primitives/base_estimator.py:371: error: Argument "observables" to "_call" of "BaseEstimator" has incompatible type "List[Union[int, SparsePauliOp]]"; expected "Sequence[int]" [arg-type] qiskit/primitives/base_sampler.py:243: error: Invalid index type "Union[int, QuantumCircuit]" for "Tuple[QuantumCircuit, ...]"; expected type "SupportsIndex" [index] qiskit/primitives/base_sampler.py:258: error: Invalid index type "Union[int, QuantumCircuit]" for "Tuple[ParameterView, ...]"; expected type "SupportsIndex" [index] qiskit/primitives/base_sampler.py:261: error: Invalid index type "Union[int, QuantumCircuit]" for "Tuple[ParameterView, ...]"; expected type "SupportsIndex" [index] qiskit/primitives/base_sampler.py:264: error: Value of type variable "SupportsRichComparisonT" of "max" cannot be "Union[int, QuantumCircuit]" [type-var] qiskit/primitives/base_sampler.py:264: error: Unsupported operand types for <= ("int" and "QuantumCircuit") [operator] qiskit/primitives/base_sampler.py:264: note: Left operand is of type "Union[int, QuantumCircuit]" qiskit/primitives/base_sampler.py:266: error: Value of type variable "SupportsRichComparisonT" of "max" cannot be "Union[int, QuantumCircuit]" [type-var] qiskit/primitives/base_sampler.py:271: error: Argument "circuits" to "_call" of "BaseSampler" has incompatible type "List[Union[int, QuantumCircuit]]"; expected "Sequence[int]" [arg-type] ```Large part of those errors can be fixed if the type cast is made explicit (I've added todo in corresponding places).
Edit (04.10.22): This fixes all remaining
primitives
errors.