Raise error for missing bounds in optimizer SNOBFIT

[cpossel]

Summary

Raise error for invalid Nones in bounds (i.e. missing bounds) of optimizer SNOBFIT.
Closes #8580

Details and comments

Function signature is misleading, allowing None for bounds. The resulting error is non-intuitive. So, this PR adds raising a more concise error message. This includes also None values within the bounds list (can happen e.g. with ansatz UCCSD).

[qiskit-bot]

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:

• @Qiskit/terra-core
• @manoelmarques
• @woodsp-ibm

[Cryoris]

Thanks for the contribution @cpossel! Could you add a test that checks this error is correctly raised? You could add it to
https://github.com/Qiskit/qiskit-terra/blob/main/test/python/algorithms/optimizers/test_optimizers.py

and check e.g. the following for an example of how to check that a code raises an error:
https://github.com/Qiskit/qiskit-terra/blob/2eee56616d50a9e26756f855ef4aa0135920ad78/test/python/algorithms/test_entangler_map.py#L51

[coveralls]

Pull Request Test Coverage Report for Build 3054844486

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.0005%) to 84.376%

---

Totals
Change from base Build 3054338474:	0.0005%
Covered Lines:	58175
Relevant Lines:	68947

---

💛 - Coveralls

[cpossel]

@Cryoris Test for failing SNOBFIT is added.
I didn't add any specific succeeding SNOBFIT test as it should already be captured (more or less) by the general scipy optimizer tests in the test suite.

[cpossel]

I need to add the scikit-quant package to the requirements file in order to fix the error qiskit.exceptions.MissingOptionalLibraryError: "The 'scikit-quant' library is required to use 'SNOBFIT'. You can install it with 'pip install scikit-quant'." in the terra testing environment. Should it be added to the requirements-dev.txt file? Or is there another location to add this requirement to?

[woodsp-ibm]

There is a unit test for the scikit-quant optimizers BOBYQA, SNOBFIT, IMFIL https://github.com/Qiskit/qiskit-terra/blob/main/test/python/algorithms/optimizers/test_optimizers_scikitquant.py but I don't think we have ever run it in CI. Any additional test should be in this file.

The documentation notes this on scikit-quant:

https://github.com/Qiskit/qiskit-terra/blob/a0db5817ac39222abeaef647a38df491d688d221/qiskit/algorithms/optimizers/__init__.py#L87-L92

[cpossel]

The test cases in test_optimizers.py follow the logic to test the proper functioning of the optimizer itself while test_optimizers_scikitquant.py uses a holistic approach i.e. testing for the correct output of a VQE calculation with the chosen optimizer.
So, following this logic, the SNOBFIT test I want to add fits far better into test_optimizers.py. I'd also like to point out that the are already 2 scipy optimizer tests (unsing BFGS) in test_optimizers.py, namely test_scipy_optimizer and test_scipy_optimizer_callback.

test_optimizers_scikitquant.py uses the RealAmplitudes ansatz which sets the bounds by itself, so the initial error can't be reproduced with the test template in test_optimizers_scikitquant.py and multiple lines of code from test_optimizers.py would need to be copied to test_optimizers_scikitquant.py what seems very redundant and unintuitive for me.

Making the test case run without installing the third party package would still be possible catching the MissingOptionalLibraryError error the same way as done in test_optimizers_scikitquant.py

[woodsp-ibm]

I am not sure I am fully following. The unit test method is just checking the bounds. All you need is to call minimize on it and pass some dummy objective function since it should fail immediately right. The run_optimizer is way more than is needed since that was done to check the optimizer could find the minimum of a function to check its correct working. Your test just checks that it fails as expected given an invalid bounds on the minimize right. To me it would be better to include that in the test suite for those 3. Yes it does use VQE - maybe finding some more straightforward function did not test the optimizers adequately. Ideally we would use as little other code as was needed to check their correctness - but I know sometimes things just don't work out.

[cpossel]

Maybe my thoughts were a little too over-complicated... With your suggestions I added a minimal test function to test_scipy_optimizer_callback.py (ignoring the pattern of the other tests with VQE). I tested it locally and it showed the intended behavior so it should be fine now.

[cpossel]

As far as I understand the output of the CI tests the test's failure is due to some CI configuration and not due to the code changes.
So, can anyone of you @Cryoris @woodsp-ibm @jakelishman look into the issue of the failed test, please?

[jakelishman]

The tests are all passing now - I see that the last commit had a test failure that's since been re-run, and it was just caused by a dodgy network connection / overallocated hardware on Azure's side. Nothing to worry about - it's just transient and we don't have control over it.

[woodsp-ibm]

Looks good, but can you add a release note for this bug fix.

[cpossel]

@woodsp-ibm Okay, I added the release note. Is anything else missing?

[woodsp-ibm]

Thanks for the contribution. All looks good to me now.