Added V2 and ISA support #197
Conversation
|
Léna Pérennès seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Pull Request Test Coverage Report for Build 10357452565Details
💛 - Coveralls |
|
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 |
qiskit_algorithms/custom_types.py
Outdated
|
|
||
| from qiskit import QuantumCircuit | ||
|
|
||
| _Circuits = Union[list[QuantumCircuit], QuantumCircuit] | ||
| _Circuits = Union[List[QuantumCircuit], QuantumCircuit] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I initially went for
_Circuits = list[QuantumCircuit] | QuantumCircuitbut 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
# Conflicts: # qiskit_algorithms/time_evolvers/pvqd/pvqd.py # qiskit_algorithms/time_evolvers/pvqd/utils.py # test/time_evolvers/test_pvqd.py
|
Hey @woodsp-ibm! Sorry for the time without any news, we've had quite a lot on our plates recently :( We just wanted to keep you posted on what's left to do. Normally, all the checkboxes that are... well, checked in the first post of this PR are ready for review. Either their tests pass, or they will once the non-checked items are done. Doing these non-checked items shouldn't be too long, it's just a matter of finding the time to do so (but hopefully soon!). Once again, sorry for that. The reason why we're pinging you even though it's not entirely finished is so that you can take a quick look at how things have been done. Since the code style is (normally) consistent between files, an error or bad coding pattern that you spot on a file will likely be repeated on others, so it's probably fine for now if you just look at "representative files" and leave comments on them, we'll just correct the other spots where we did the same mistake. Sorry again for the long time without news, but we should be close to the end! |
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: