Added V2 and ISA support #197
Conversation
|
|
|
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 |
|
I did #234 to address the CI failures so hopefully that should go through ok here in CI (it did locally for me in passing all tests). |
|
CI and the notebook now pass. However as you had changed a couple of the files, I also changed, it seems that the changes overlapped and cause a conflict. For the notebook I just edited and saved it and did not save the re-run version so that I did not get deprecation warnings in what was checked in. It did update the Python version when doing that which conflicts but having it be the version you ran things under is fine. For the unit test I guess you had changed the initial point - hopefully the one I had changed it too works fine now instead. But I'll let you resolve the conflicts here |
|
It seems with the newer primitives and the values I had found that worked across scipy 1.15.3 and 1.16.0 with the former primitives (that you can see work fine in the nightly CI actions here), that a couple of tests did not do so well with 1.16.0 here. Hopefully you can find values that pass both with 1.15.3 and 1.16.0 the newer primitives here. For me I ran the test locally uninstalling/installing scipy to get the version I wanted and trial and error finding something that worked with both - on the positive side its only a couple of tests failing so some of the changes I had to make in other tests worked out ok it seems. |
|
@woodsp-ibm |
My recollection is that I tried with SLSQP and given it computes a finite diff gradient by default, using the objective function, I think I ended up with so many callbacks and the array was large. Maybe since SPSA only does + and - around the point I guess you could try that. This test, as you can see, I could not get to work with the same values for each version rather I ended up detecting the scipy version and to get something that would work. If SPSA seems to do it that might be better as it's not from scipy so should not need version dependent code like I ended up with in the test. |
|
Since the only points that diverged were the last two ones and since the goal of this test is only to check that the callback works as expected, I reduced the number of iterations of |
|
10 is fine - it was less (3) before but its seems that the newer COBYLA does not stop on so few iters despite it seemingly being a limit ie maxiter - maybe that will change but this seems good. |
We'll get to it ASAP! By the way, I think there's also this comment on which we'd need your input! |
|
And another weird thing: the CLA complained that some commits done by Léna weren't associated to an email address that signed the CLA when the PR was still a draft, but doesn't seem to complain now that we've put it ready for review, is it expected? |
That code has been around a long time - I don't know. @Cryoris might you have any idea? One thing would be to set the algorithm_globals.random_seed so that the test always uses the same values. |
I think the CLA is for qiskit projects overall so could they have signed it on another one. If CLA passes it all seems good to me. |
|
@woodsp-ibm We've added the release note, let us know if that's not what you expected! |
|
I am thinking that perhaps adding a link to this migration guide in the release notes might be helpful https://quantum.cloud.ibm.com/docs/en/migration-guides/v2-primitives It not only covers the runtime ones but also has a link to the v1 V2 migration for reference primitives in Qiskit. Do you think an Estimator sample of before with V1 and now with V2 would be helpful as well, alongside the Grover Sampler one. That would cover both primitive types and their usage in algorithms from a migration perspective. One minor text item
Would this be better i.e.
Overall though it seems fine to me. |
|
We've updated the release note and also removed a section in another release note stating that |
| :class:`~qiskit.circuit.QuantumCircuit` or a list thereof and | ||
| additional options and returns the transpiled version of its input(s) | ||
| according to the provided options. | ||
| features: |
There was a problem hiding this comment.
I am thinking perhaps, to make it self evident here, that adding a paragraph that says other algorithms that use a user provided quantum circuit, such as the ansatz for VQE, if the circuit requires transpiling for the underlying device that the primitive is using, then the user should externally transpile the circuit and pass in a transpiled circuit. The algorithms could be listed here too.
(I must admit I had wondered for consistency, even though a user can transpile the circuit themselves in these cases, whether having transpilation capability like the other algorithms might be something. Had you considered that at all - i.,e have all algos be consistent even though in cases like VQE its not strictly needed. I imagine if not transpiled though you'll end up with an error from the underlying system, which I guess can happen anyway too with the algos that have the transpile capability if you fail to use it when needed.)
There was a problem hiding this comment.
(I must admit I had wondered for consistency, even though a user can transpile the circuit themselves in these cases, whether having transpilation capability like the other algorithms might be something. Had you considered that at all - i.,e have all algos be consistent even though in cases like VQE its not strictly needed. I imagine if not transpiled though you'll end up with an error from the underlying system, which I guess can happen anyway too with the algos that have the transpile capability if you fail to use it when needed.)
I can hear both arguments. I would probably choose adding the transpilation option to all cases, so that the user doesn't have to transpile the ansatz by themselves, add so that it's a bit more consistent across the package: if you want to run a circuit on a quantum computer, you have to pass a transpiler. For some algorithms, if you transpile it yourself beforehand it would work, but the package doesn't guarantee it.
Tell us what you'd rather have and we'll either change the release note or the code!
There was a problem hiding this comment.
For me I think it would be helpful to have consistency so that way if things need to be transpiled the way to do it is the same. Sure with something like VQE its possible to do it on the circuit that is passed, which you would still be able to do if you wished even with transpilation options in its API. But having it the same/consistent for all algorithms I feel would be beneficial overall for an end user - especially if they are not so familiar with the algorithms workings.
There was a problem hiding this comment.
Just to be sure, in that case, how would that go for classes that can be called from other ones? I'm especially thinking about the gradients and VQE here, since the last one can be called from AdaptVQE. Since they can be called in a standalone manner, they would have to support transpilation options. For now however, the transpiler and its options are private attribute without getter/setter, which means that it shouldn't really be accessed from another class. What is the best course of action here?
- Write getters/setters for all transpiler attribute, and for each class like this:
- Check if the subclass has a transpiler
- If yes, and the current class doesn't have one, set the current class' transpiler to the subclass'
- If no, and the current class does have one, set the subclass' transpiler to the current class'
- Specify that in such a case, both the subclass and the parent class must be provided with a transpiler.
I'd say that the latter option is more verbose/understandable from a user point of view, though a bit less convenient than the first one. What would you rather have here?
There was a problem hiding this comment.
I would agree that the latter option seems to be the one for now - perhaps there is room for optimization at some point in the future if the more verbose way seems to need it. If each item can be used standalone, if it needs to transpile circuits as it going to do execution itself then I agree the easiest option is to have the transpilation on each item. This would also allow one to do different transpiles if that has any value which trying to pass options on down would not.
When you talk about AdaptVQE does it need any transpilation option itself or is that all done by VQE as its the one doing the execution. I do see AdaptVQE is direct setting the VQE ansatz public attribute, which I guess anyone can. I imagine if it changed that would need to trigger a re-transpilation assuming the transpiled circuit is cached.
There was a problem hiding this comment.
While adapting the code to include this change, I noticed that aux_operators is treated differently in VQE and in VQD. In VQE, one simply has
if aux_operators is not None:
aux_operators_evaluated = estimate_observables(
self.estimator,
self.ansatz,
aux_operators,
optimizer_result.x, # type: ignore[arg-type]
)
else:
aux_operators_evaluated = Nonewihle in VQD more checks are performed
if aux_operators:
zero_op = SparsePauliOp.from_list([("I" * self.ansatz.num_qubits, 0)])
# Convert the None and zero values when aux_operators is a list.
# Drop None and convert zero values when aux_operators is a dict.
key_op_iterator: Iterable[tuple[str | int, BaseOperator]]
if isinstance(aux_operators, list):
key_op_iterator = enumerate(aux_operators)
converted: ListOrDict[BaseOperator] = [zero_op] * len(aux_operators)
else:
key_op_iterator = aux_operators.items()
converted = {}
for key, op in key_op_iterator:
if op is not None:
converted[key] = zero_op if op == 0 else op # type: ignore[index]
aux_operators = converted
else:
aux_operators = NoneWhat's the reason for that? In particular, since I need to iterate over aux_operators to apply the ansatz layout to the operators, I end up writing a very similar code for VQE, including defining a zero_op placeholder for instance (see the last commit). Is that expected? Since I don't know what's the purpose of these aux_operators, I'm a bit lost as to what should I do with them.
In particular, contrarily to VQD, VQE does not drop the None values, and I'm not really sure whether I should add this or not.
There was a problem hiding this comment.
The aux_operators were often used in Nature where for instance we would be computing the ground state of the molecule using the main electronic operator but the molecule has other properties that can be measured in that ground state - where some for instance the number of particles operator can be used as check for that state since that would be a known quantity. I recall operators being X,Y and Z dipole operators, the number of particles operator and magnetization. I seem to recall 6 but hopefully that gives the idea - extra operators we just compute the expectation values of given the state found.
ListOrDict type says the type in the List is Optional so None is allowed. My recollection was that the aux operators, which the user could supply, in the process of computation we may, for example in Nature find symmetries of the molecule which using a symmetry operation we could then reduce the size of the main operator without losing an precision (i.e. remove the symmetries from it to make it smaller). However while this symmetry may hold for the main operator it might not for an aux_operator, So if it did not commute with symmetry we would have to remove it as the size of the main operator, being reduced by symmetries, would no longer match so we could not use the resultant state to estimate that aux_operator expectation value. In the list they were positional so to maintain the positions down and back up the stack when processing, so as things ended up back to the user as expected, Nones were allowed in there. Part of the reason a Dict form was added later that the key/value pairs were easier to drop and still understand what the dict had in it.
Why one handles it better than the other I am not sure. Things have been changed quite a bit in algorithms as things evolved. Does that help at all?
Summary
This PR's goal is 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_estimatorsgroverphase_estimatorsvalidationStill to be done: