Fix mypy errors (algorithms) #8271
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:
|
HuangJunye
added
the
Community PR
|
I've rebased the PR on master |
|
This PR is much bigger now due to the |
qiskit/algorithms/amplitude_amplifiers/amplification_problem.py
Outdated
Show resolved
Hide resolved
| self._circuit_results = None | ||
| self._max_probability = None | ||
| self._oracle_evaluation: bool | None = None | ||
| self._circuit_results: list[np.ndarray] | list[dict[str, int]] | None = None |
There was a problem hiding this comment.
This annotation is duplicated in the return of circuit_results method. You should make a type alias CircuitResultsT = list[np.ndarray] | list[dict[str, int]] | None. And I think you have to use Optional and Union for the type alias declaration. Because it is not supported yet until Python 3.10.
There was a problem hiding this comment.
Actually, you should exclude the | None in the CircuitResultsT declaration, because the circuit_results setter doesn't expect None.
qiskit/algorithms/amplitude_amplifiers/amplification_problem.py
Outdated
Show resolved
Hide resolved
qiskit/algorithms/amplitude_amplifiers/amplification_problem.py
Outdated
Show resolved
Hide resolved
qiskit/algorithms/amplitude_amplifiers/amplification_problem.py
Outdated
Show resolved
Hide resolved
|
I have reviewed all of the |
|
I commented in the #8269 one for opflow that algorithms have been refactored over to use the new primitive based execution paradigm and away from opflow. So since this was started there has been quite a bit of change. Is this something you wish to complete. While opflow is being completely deprecated parts of the algorithms are, linear solvers (hhl), factorizers (shor) are deprecated and minimum_eigen_solvers, eigen_solvers and evolvers are pending deprecation now and will be deprecated shortly. The latter are replaced by primitive based equivalents in minimum_eigensolvers, eigensolvers (no _ between eigen and solvers in the new names) and time_evolvers respectively. Amplitude amplifiers, amplitude estimators and phase estimators are refactored in place and take either QuantumInstance and use opflow, or take/use a Sampler (primitive) with the former, now pending deprecated, and will be deprecated leaving them only taking a primitive. So some files, those that will be removed, like opflow, we could consider not changing them - it would be complete folders for minimum_eigen_solvers, eigen_solvers, evolvers, linear solvers and factorizers. Some work has already been done there and I do not know what might be needed to complete it. Either way that code will be removed in the future. The rest will remain so if you are interested in completing this I guess there is still quite a bit to do - especially since quite a bit of new code has been added since this work was started. |
woodsp-ibm
added
the
mypy
|
This was something we discussed originally when creating PRs, that unless typechecks are CI'd there will be overhead of added untyped code. I'm well aware of it. I can rebase on master whenever there are plans to review this PR. I will also add fixes for obvious stuff in new code. Anyway there we no plans to fix everything in one PR, only add fixes to whatever can be and try to discuss the less obvious issues. |
|
So @woodsp-ibm can you please review the PR? |
qiskit/algorithms/amplitude_amplifiers/amplification_problem.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Julien Gacon <gaconju@gmail.com>
|
@Cryoris @woodsp-ibm @ElePT do you mind to review? I know this PR is large, but that's precisely why it is hard to keep it up-to-date. |
| @@ -294,7 +298,7 @@ def amplify(self, amplification_problem: AmplificationProblem) -> "GroverResult" | |||
| raise AlgorithmError("Sampler job failed.") from exc | |||
|
|
|||
| num_bits = len(amplification_problem.objective_qubits) | |||
| circuit_results = { | |||
| circuit_results: dict[str, Any] | Statevector | np.ndarray = { | |||
There was a problem hiding this comment.
This is just FYI, the quantum instance is deprecated and will be removed in following qiskit releases. Once the quantum instance is removed, I believe that the circuit_results will always be a dict[str,float], because the type of results.quasi_dists in the line below is fixed. This is not the only annotation that will be simplified, so I can keep track of these when we open the removal PR.
| @@ -13,7 +13,7 @@ | |||
| """The Maximum Likelihood Amplitude Estimation algorithm.""" | |||
|
|
|||
| from __future__ import annotations | |||
| import typing | |||
| from typing import Sequence, Callable, List, Tuple | |||
There was a problem hiding this comment.
I remember this working for normal typehints, but failing for defining custom types in 3.7:
>>> from __future__ import annotations
>>> A = list[float]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: 'type' object is not subscriptable
I think the issue here is line 32. We ran into a similar problem in Nature and just reverted to typing, I don't know if there is any good workaround.
qiskit/algorithms/amplitude_amplifiers/amplification_problem.py
Outdated
Show resolved
Hide resolved
* Fix mypy errors (algorithms) * Fix formating Co-authored-by: Julien Gacon <gaconju@gmail.com> * Remove ignores, fix imports and a couple of small bugs * More fixes * Update qiskit/algorithms/minimum_eigen_solvers/vqe.py Co-authored-by: Julien Gacon <gaconju@gmail.com> * More fixes * Remove TODO * Fix `eval_observables` with `Backend` parameter * Fix some more annotations * Fix some TODOs * Get rid of typing imports where possible * Fix annotation * Fix wrong result population * Fix result types for VQD * Fix lint * Remove trailing mypy comment --------- Co-authored-by: Julien Gacon <gaconju@gmail.com>
* Fix mypy errors (algorithms) * Fix formating Co-authored-by: Julien Gacon <gaconju@gmail.com> * Remove ignores, fix imports and a couple of small bugs * More fixes * Update qiskit/algorithms/minimum_eigen_solvers/vqe.py Co-authored-by: Julien Gacon <gaconju@gmail.com> * More fixes * Remove TODO * Fix `eval_observables` with `Backend` parameter * Fix some more annotations * Fix some TODOs * Get rid of typing imports where possible * Fix annotation * Fix wrong result population * Fix result types for VQD * Fix lint * Remove trailing mypy comment --------- Co-authored-by: Julien Gacon <gaconju@gmail.com>
* Fix mypy errors (algorithms) * Fix formating Co-authored-by: Julien Gacon <gaconju@gmail.com> * Remove ignores, fix imports and a couple of small bugs * More fixes * Update qiskit/algorithms/minimum_eigen_solvers/vqe.py Co-authored-by: Julien Gacon <gaconju@gmail.com> * More fixes * Remove TODO * Fix `eval_observables` with `Backend` parameter * Fix some more annotations * Fix some TODOs * Get rid of typing imports where possible * Fix annotation * Fix wrong result population * Fix result types for VQD * Fix lint * Remove trailing mypy comment --------- Co-authored-by: Julien Gacon <gaconju@gmail.com>
* Fix mypy errors (algorithms) * Fix formating Co-authored-by: Julien Gacon <gaconju@gmail.com> * Remove ignores, fix imports and a couple of small bugs * More fixes * Update qiskit/algorithms/minimum_eigen_solvers/vqe.py Co-authored-by: Julien Gacon <gaconju@gmail.com> * More fixes * Remove TODO * Fix `eval_observables` with `Backend` parameter * Fix some more annotations * Fix some TODOs * Get rid of typing imports where possible * Fix annotation * Fix wrong result population * Fix result types for VQD * Fix lint * Remove trailing mypy comment --------- Co-authored-by: Julien Gacon <gaconju@gmail.com>
Summary
Following the discussion, I'm splitting #8187 by module.
Details and comments
There are ~475 errors left. This is the module with the most type errors so far.
(UPD 04.10.22: I've rebased on the current master. There appear to be many new errors)
All error list