Skip to content
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

Improve typing #74

Open
12 tasks
mrossinek opened this issue Sep 8, 2023 · 3 comments
Open
12 tasks

Improve typing #74

mrossinek opened this issue Sep 8, 2023 · 3 comments

Comments

@mrossinek
Copy link
Member

mrossinek commented Sep 8, 2023

What should we add?

While working on #73, @ElePT and I have identified the following inconsistencies and/or problems around the type hints which should be investigated further. Rather than holding up that PR unnecessarily, we decided to gather these here.
That way, they can be investigated one-by-one.

Note: some of these may very likely require code changes to resolve inconsistencies in the interfaces.

  • Typing around np.ndarray vs Sequence[float]. It is a known issue that to mypy the numpy arrays do not satisfy the Sequence type. This leads to many type ignore statements right now which can hopefully be improved upon.
    • For example, typing around parameter values being passed on to the primitives: sometimes its Sequence[float], sometimes its Sequence[Sequence[float]], and oftentimes internally it ends up as np.ndarray causing many type ignore comments
  • The typing of the initial_point attributes is very inconsistent across its various occurrences.
  • The Minimizer interface does not appear to be supported properly by VQD and PVQD (for example) since they do not provide the required jac argument
  • The ListOrDict type is causing quite some oddities. This has multiple cases:
    • for the metadata (e.g. variance) types and implementations are inconsistent with the variance sometimes wrapped in dictionaries and sometimes provided as a single number
    • for aux_operators and/or observables we had to add many type ignores which would need to be re-evaluated (possibly linked to the previous point)
  • All classes that use the EsimationProblem have very poor typing around the following points:
    • the post_processing function
    • the confidence intervals
    • the circuit_results
  • Add py.typed file back again - removed by Remove py.typed until #74 is addressed #121
@woodsp-ibm
Copy link
Member

woodsp-ibm commented Sep 8, 2023

The Estimation logic - and possible elsewhere - has code that is no longer used. While deprecated code for QI/opflow was removed there are some util like functions taking types that would have been an outcome - this code is now untested.

Also Grover has some issue around using the circuit passed a as grover operator - it allows QuatumCircuit but then treats it as GroverOperator where it unconditionally accesses an attribute that has which is not present on a QuantumCircuit. Update: I raised #76 for this.

This was just from starting to look at mypy in AA and AE folders.

@woodsp-ibm woodsp-ibm reopened this Sep 8, 2023
@mrossinek
Copy link
Member Author

I suppose a good strategy at finding which of the code is unused would be to look at the coverage reports. Obviously not all uncovered lines can be removed but it might be a good starting point 👍

@woodsp-ibm
Copy link
Member

Yes, particularly around the algos AE, Grover and Phase Estimation that were modified to add Sampler logic paths to the existing code and whether more could/should have been removed when QI paths were removed that was part of common utilities.

woodsp-ibm added a commit to woodsp-ibm/qiskit-algorithms that referenced this issue Jan 11, 2024
mergify bot added a commit that referenced this issue Jan 17, 2024
Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants