-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Allow list of optimizers+initial points in VQD
#9151
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Supporting initial point being a list is something we recognized was lacking when these were recently refactored to primitives. This implements VariationalAlgorithm which defines the property and that interface is used elsewhere. We had discussed perhaps a separate interface for ground versus excited state eigensolvers use. When plotting dissociation curves in Nature adjusting the molecule a little, the prior solution point often is better than a random start point, so setting them all is highly desirable. Now separate optimizers - that seems more complicate for a user to deal with - do you have some specific use case - maybe its the same optimizer class but parameterized differently? What do you find that set differently? @ElePT as you have done some work with the VQD can I assign you to this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @ariannacrippa! I have one question below, and otherwise some minor comments 🙂
if step > 1: | ||
prev_states.append(self.ansatz.bind_parameters(result.optimal_points[-1])) | ||
if isinstance(self.initial_point, list): | ||
params = validate_initial_point(self.initial_point[step - 1], self.ansatz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood VQD correctly, our cost function for parameters x
, an ansatz phi
is
L(x) = <phi(x)|H|phi(x)> + penalty * ( |<phi(groundstate)|phi(x)>|^2 + |<phi(1st_excited)|phi(x)>|^2 + ...)
where groundstate
are the parameters for the ground state and 1st_excited
for the first excited etc. So the prev_states
variable here I think should always be the optimal points.
The initial points in Qiskit are usually understood as starting points for the optimizations, so at iteration k
of the VQD for the k-th
excited state, element k
of the initial_points
list should be the initial point of the optimization. Instead here, I think you are implementing
L(x) = <phi(x)|H|phi(x)> + penalty * ( |<phi(initial_points[0])|phi(x)>|^2 + |<phi(initial_points[1])|phi(x)>|^2 + ...)
or maybe I misunderstood the code 🤔
If you indeed need the latter, then we can also discuss adding that, but I think the initial_points
should only act as starting points for the optimization and not be part of the loss function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment. Yes you are right, what I want to do is use the previous set of parameters as in L(x) = <phi(x)|H|phi(x)> + penalty * ( |<phi(groundstate)|phi(x)>|^2 + |<phi(1st_excited)|phi(x)>|^2 + ...)
but put in x
a given set of parameters from the list of the initial points and then do the optimization. The local file that I was working on was slightly different from the version that I then modified and uploaded. I have to check better on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ariannacrippa! It looks like a few unit tests are failing in the CI (automatic test), in particular: test_aux_operators_std_dev
and test_callback
.
The first test fails because in line 63 you are checking len(point)
and if point
is a float
, it has no length. (see line 17471 here: https://dev.azure.com/qiskit-ci/qiskit-terra/_build/results?buildId=41666&view=logs&j=e4ea27c2-3a16-5b32-7be8-0bf30fe9e828&t=793d4f07-8490-552a-798f-6a5657042d16).
The second test fails for an assertion error, it looks like the values don't match the expected results anymore (see line 17516 from link above).
I was considering working on and opening a similar pull request, but since this one already exists I have one suggestion to add (Unless someone thinks this would merit a separate pull request): The current VQD implementation appears to require that the same ansatz is used for all excited states. However, there is nothing in the VQD formulation that requires this to be the case. The ground state ansatz could be UCCSD and the first excited state could be RealAmplitudes and that would still be valid. (Although I would find this particular choice a little strange.) A couple of more realistic examples of how allowing the ansatz to be different across different excited states could be useful (In addition to allowing the initial parameters to be different):
|
Pull Request Test Coverage Report for Build 4699380082
💛 - Coveralls |
This change looks good to me, thanks @ariannacrippa! Could you add a releasnote and a test? @JoelHBierman sounds like a good idea, let's do that in a follow-up! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Arianna!!
It looks like your release note is in qiskit-terra.qiskit.algorithms.eigensolvers.releasenotes.notes
, but it should be in the top directory. Can you please move it to qiskit-terra.releasenotes.notes
? (and delete the releasenotes folder you created inside eigensolvers?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello again, I left some typehint-related suggestions, but apart from that, the code looks good to me. All that's missing to approve this PR is adding a test for multiple optimizers to the VQD unit test, you can find the file here. If you have any question about how to write a unit test, you know where to find me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but @Cryoris, could you take a final look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small note on the reno, otherwise LGTM, thanks for bringin it over the finish line!
releasenotes/notes/vqd-list-initial-points-list-optimizers-033d7439f86bbb71.yaml
Outdated
Show resolved
Hide resolved
…d7439f86bbb71.yaml
* test commit * added optional lists of init ial point and optimizer * fixed initial points and add more compact expressions * fixed bug initial_point input * fix initial points * formatting * fix none initial point * Added releasenote * Ensure initial_point sampled once only if None * moved release note * Update typehints * Update typehints * Update typehints * Update typehints * Update typehints * Apply suggestions from code review * Fix conflict, add test * Fix black * Update releasenotes/notes/vqd-list-initial-points-list-optimizers-033d7439f86bbb71.yaml --------- Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: ElePT <[email protected]> Co-authored-by: ElePT <[email protected]>
* test commit * added optional lists of init ial point and optimizer * fixed initial points and add more compact expressions * fixed bug initial_point input * fix initial points * formatting * fix none initial point * Added releasenote * Ensure initial_point sampled once only if None * moved release note * Update typehints * Update typehints * Update typehints * Update typehints * Update typehints * Apply suggestions from code review * Fix conflict, add test * Fix black * Update releasenotes/notes/vqd-list-initial-points-list-optimizers-033d7439f86bbb71.yaml --------- Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: ElePT <[email protected]> Co-authored-by: ElePT <[email protected]>
* test commit * added optional lists of init ial point and optimizer * fixed initial points and add more compact expressions * fixed bug initial_point input * fix initial points * formatting * fix none initial point * Added releasenote * Ensure initial_point sampled once only if None * moved release note * Update typehints * Update typehints * Update typehints * Update typehints * Update typehints * Apply suggestions from code review * Fix conflict, add test * Fix black * Update releasenotes/notes/vqd-list-initial-points-list-optimizers-033d7439f86bbb71.yaml --------- Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: ElePT <[email protected]> Co-authored-by: ElePT <[email protected]>
* test commit * added optional lists of init ial point and optimizer * fixed initial points and add more compact expressions * fixed bug initial_point input * fix initial points * formatting * fix none initial point * Added releasenote * Ensure initial_point sampled once only if None * moved release note * Update typehints * Update typehints * Update typehints * Update typehints * Update typehints * Apply suggestions from code review * Fix conflict, add test * Fix black * Update releasenotes/notes/vqd-list-initial-points-list-optimizers-033d7439f86bbb71.yaml --------- Co-authored-by: Julien Gacon <[email protected]> Co-authored-by: ElePT <[email protected]> Co-authored-by: ElePT <[email protected]>
Summary
During my research, I needed to be more flexible with the VQD algorithm, e.g. I wanted to be able to choose an arbitrary initial point for my set of eigenstates. Especially when I already have knowledge of the system that I am simulating. In addition, I found it helpful to have the possibility to decide which optimizer to use for each kth eigenvalue and be able to define the number of iterations for every kth step. For example, in some cases, I needed more iteration for excited states than the ground state.
For these reasons, I have added the following:
-input for initial point: single or list. If single, the kth excited state is computed from the previous optimal point (already implemented). If list, every kth eigenvalue is computed starting from the initial points in the list.
-input for optimizer: single or list: If single, already implemented. If list, every kth eigenvalue is computed with kth optimizer.
Details and comments
I tried the code with NFT and COBYLA optimizers. However, when I tried with SPSA I obtained an error.