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

Added V2 and ISA support #197

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Added V2 and ISA support #197

wants to merge 11 commits into from

Conversation

tnemoz
Copy link
Contributor

@tnemoz tnemoz commented Aug 9, 2024

Summary

This PR is a work in progress to adapt the whole code base to support V2 primitives and ISA circuits. Will fix #136, fix #164, fix #165, fix #194, and fix #204.

Details and comments

For now, it contains:

  • phase_estimators
  • eigensolvers/vqd.py
  • amplitude_amplifiers/grover.py
  • time_evolvers
  • state_fidelities/compute_uncompute.py
  • optimizers
  • observables_evaluator.py
  • gradients
  • minimum_eigensolvers
  • amplitude_estimators

@CLAassistant
Copy link

CLAassistant commented Aug 9, 2024

CLA assistant check
All committers have signed the CLA.

@tnemoz tnemoz mentioned this pull request Aug 9, 2024
@coveralls
Copy link

coveralls commented Aug 9, 2024

Pull Request Test Coverage Report for Build 10357452565

Details

  • 24 of 25 (96.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 90.448%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_algorithms/custom_types.py 5 6 83.33%
Totals Coverage Status
Change from base Build 10319023971: 0.004%
Covered Lines: 6382
Relevant Lines: 7056

💛 - Coveralls

@woodsp-ibm
Copy link
Member

woodsp-ibm commented Aug 9, 2024

FYI. I merged a PR that fixed CI with the main branch and changes that will going into the shortly to be released 1.2.0 so that CI now passes fully here and does not fail with the two jobs that check things out against qiskit main, and updated the branch here so this now completely passes.

I think this is fine. I must admit though I had wondered, given that I think we only ever need something that has a run() method, that takes circuits and gives transpiled circuits back whether, something could be done around a type that was more targeted along those lines. The pass manager has a number of methods that I do not think we would call, and the AI Transpiler Service which has a run method could be used too but is not a passmanager - i,e, so that a Passmanager instance or the AI transpiler could be used - or anything else that had a run method that did the transpilation. I know Optimization went for BassPassManager too as the type and maybe thats ok.

Whatever is done I think we might want to note in the docstring for this that's its optional and that passing in None results in the circuit not being transpiled. Maybe for some algos like VQE one can already pass a circuit (ansatz) that is transpiled for a given backend so the note there might be a little different to accommodate that - for PE they build their own circuits of course so that aspect does not apply.

One minor comment I might have is that the pm used in the tests is created and ends up being used for all the tests rather than being created at the time the data is passed into the test. As long as there are no side-effects of using pm, i.e. its the same really as creating a new pm for each test run I guess its ok - my feeling is that it would be better to do it each time so it guarantees a fresh instance is used.


from qiskit import QuantumCircuit

_Circuits = Union[list[QuantumCircuit], QuantumCircuit]
_Circuits = Union[List[QuantumCircuit], QuantumCircuit]
Copy link
Member

@woodsp-ibm woodsp-ibm Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use list in 3.8 - which arguably is better and what we have tried to do elsewhere. You do need to add a future import to have that work from __future__ import annotations. You can see examples in this repo e.g. in this file which does that and uses it https://github.com/qiskit-community/qiskit-algorithms/blob/main/qiskit_algorithms/time_evolvers/pvqd/pvqd_result.py Though I am not sure here with custom types defs though as I see Union and elsewhere we try and use the | nowadays instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially went for

_Circuits = list[QuantumCircuit] | QuantumCircuit

but then make lint complained with:

************* Module qiskit_algorithms.custom_types
qiskit_algorithms/custom_types.py:21:12: E1131: unsupported operand type(s) for | (unsupported-binary-operation)

which is why I went for Union instead of |. Or is there a workaround I don't know of?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That future import is needed for | as well - but if you had that import then as I mentioned I was not not sure here as I recall having issues when defining types before using these newer aspects where we had to have them defined with the former Typing constructs. If you search in this repo you will find just a few Union hits where they are pretty much limited to being used in type defines like this - I also see List being used in the ones here so it may well be the same issue with that. I commented mostly as a saw the commit that changed things to fix it for lint in 3.8 from list to List that was all the change I did not see a from future import being removed which is needed to type things this newer way in 3.8.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One further comment around 3.8. Qiskit has already deprecated support for Python 3.8 and support will be removed in 1.3.0 which is due in Nov. Python 3.8 EOL is Oct this year so among the changes here it could be a choice to drop 3.8 support too along with these changes. To drop this would evidently also include dropping it from CI tests etc and bumping things to run at 3.9 min.

In talking about versions I will note 3.13 is planned to be available beginning of Oct. There is an issue on Qiskit Qiskit/qiskit#12903 to support this so again depending on timeline.... but hopefully that's just adding it as supported and to CI to test when dependencies are there and it just works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants