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!
…or Grover and see if we can get something better for pVQD
|
In reference to #197 (comment) above For _circuit_key. Maybe a simple expedient for now, rather than removing the functionality, is to copy the code over that was there for _circuit_key and make a local function here https://github.com/Qiskit/qiskit/blob/stable/1.4/qiskit/primitives/utils.py called circuit_key() and use that As far as I can see that function uses public attributes of QuantumCircuit all except this line
but from QuantumCircuit code I see this, which says there is a public attribute added so no need to access the private member.
FYI here is some link to an original discussion around circuit_key and it references the issue it was addressing Qiskit/qiskit#8604 I will note Qiskit Machine Learning has copied some code over from Algs here that it was dependent upon, including code depending on that circuit key, so they will end up having the same issue. I'll reference the issue for their 2.0 migration which is qiskit-community/qiskit-machine-learning#897 As to what to perhaps do instead, if this expedient would be done but wanted to be removed going forwards, maybe its worthy of opening a separate Issue to propose solution/discuss? |
|
@woodsp-ibm Thanks for the resources and the guidance! We'll implement our own |
|
In looking at the todo list above I think this item
might be better in a separate PR so as this PR is focused around the primitives change and hopefully that will make it easier for reviewers as well. The circuit_key change can be included here though as that's needed to pass unit tests under Qiskit 2.0 - anything else that fails too. But changing over the circuits constructors to the functions, and aspects around that which need updating as a consequence of this change I feel would be better in another PR. I had opened an issue #231 around this and made a comment in there around behavior change of num_qubits and lack of setting once circuits like TwoLocal are finally removed from Qiskit. |
|
FYI: In case it helps and you were not aware... in the root of the project is a make file with several targets, |
I am aware! Actually, this is what I've used to correct the linting errors in local. For the subsequent runs, I just always forget to rerun them 😅 Concerning the test suite however, I do run it in local beforehand, but it's at the exception of the Is there a problem I'm not aware of with this approach (like if there's a monthly rate of workflows GitHub can run on a given repository, or if you receive a notification for each failed run)? |
|
No that's fine. With seeing the copyright failure I thought I'd mention just in case. I often do the same - fix something that comes up quick like and forget to run say the copyright check locally only to have it fail in the build on github! 🤣 |
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.
What's working
eigensolversgradientsminimum_eigensolversoptimizersstate_fidelitiestime_evolversutilsamplitude_estimatorsgrovertweedledumisn't needed anymore for Qiskit 2.0.phase_estimatorsvalidationStill to be done:
_circuit_keyas it's not present in Qiskit 2.0 anymore.