Adding a convergence threshold to VQD to filter non-sensible values

[tnemoz]

Summary

As mentioned here, some optimizers don't work well with shots-based samplers. This leads to VQD returning sometimes absurd values, as shown here.

This Pull Request adds a convergence_threshold to VQD to test whether the optimization converged well.

It also paves the road for adapting VQD to V2 primitives.

Details and comments

Changes that were made:

• a convergence_threshold parameter now tests whether the average fidelity of the $k$-th eigenstate w.r.t. the previously found eigenstates is larger than this threshold, indicating that not only the eigenvalue returned is likely to be false, but the next iterations won't make sense either.
• if the convergence_threshold test passes, VQD now returns $\left\langle\psi_k\middle|H\middle|\psi_k\right\rangle$ instead of $\left\langle\psi_k\middle|H\middle|\psi_k\right\rangle+\sum\limits_{i=1}^{k-1}\beta_i\left|\left\langle\psi_k\middle|\psi_i\right\rangle\right|^2$, which not only makes more sense, but also allows to get decent values even when using gradients-based optimizers such as SLSQP.
• the tutorial has been updated to use COBYLA and a value of betas that are closer to real-world use cases.
• the tests have been updated. In particular, the values of betas have been updated from 50 to 3. A too large value with shot noise made the optimizers focus solely on the fidelity part, forgetting about the energy part.

[tnemoz]

Probably a silly question, but why does make lint succeeds in 3.8 and not in others? It also runs without error on my machine, where I use 3.12.

Is it preferable to correct the linting by adding ignores in the concerned files before merging, even though they're not related to this PR?

[woodsp-ibm]

...why does make lint succeeds in 3.8 and not in others? It also runs without error on my machine, where I use 3.12.

From looking at the logs the nightly builds started failing this way a couple of days ago. Usually new failures like this happen when a new version pf pylint is released. Which indeed seems to have been the case with 3.3.0 having been released Sep 20th. Now this version supports Python 3.9 and above and why the 3.8 passes is that it still pulls in the older 3.2.7 which is the newest one supported under 3.8. Why it passes locally for you is that you are probably using a version of pylint prior to 3.3.0 - if you install that I imagine it will fail for you too.

Is it preferable to correct the linting by adding ignores in the concerned files before merging, even though they're not related to this PR?

As this is really independent of this PR what we normally do is to do a PR to fix things for CI when new versions of tools give new errors. So a new PR just to get CI running again would be preferable. Once that is merged this would pass and so would nightly CI too (you can see these runs under Actions in the main toolbar above - the Scheduled ones are the nightly CI runs.

I think disabling the error is the "right" way for now. In new code we tried to limit the positional args via designating that the remainder were keyword only. But I imagine doing this on these older places is likely to break code as I see public APIs like the init among the places being flagged (but I did not look extensively)

[woodsp-ibm]

FYI As you mention builds under 3.8 I raised this issue not so long ago

• Python 3.9 is now Qiskit minimum level supported #201

Probably for an upcoming release the minimum version here should be bumped to 3.9 and 3,8 removed from CI (or adjusted up to 3.9 where its the testing the lower bound - e.g we have some that test at min and max versions of Python supported). 3.8 EOL is end October so not so far off. Either way any change in this regard would be a separate PR.

[coveralls]

Pull Request Test Coverage Report for Build 11436312867

Details

• 23 of 27 (85.19%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.01%) to 90.432%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit_algorithms/eigensolvers/vqd.py	23	27	85.19%

Totals
Change from base Build 11349771169:	-0.01%
Covered Lines:	6389
Relevant Lines:	7065

---

💛 - Coveralls

[tnemoz]

@woodsp-ibm Is there anything left to do on my part to merge this? I'd understand if this needed multiple reviews since this is a larger code modification than just adding some pylint: ignore of course! I just want to be sure that I didn't forget something that could block the process 🙂

[woodsp-ibm]

I looked over it and all it all seemed fine so I approved it. I guess I normally let the originator of any PR merge it so I did not do that myself. In terms of number of reviewers we have it in the settings set to just one so thats why, with just my approval, it shows up as ready to merge and this is all we have needed. So please go ahead.

[tnemoz]

@woodsp-ibm Whoops, didn't know I could do that, sorry! Sorry if that's a newbie question, but how do I do that? GitHub tells me that only those with write access can merge PRs. This is what I see on my side:

And the only other action it seems I can take is "Close with comment". Is there a paricular interface I should be aware of, or is there a problem with my rights?

[woodsp-ibm]

Ok. sorry my bad then. I had not realized that even if approved you could not merge it as originator. I'll do it.