Skip to content
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

Default bounds for UCC ansatz circuits #818

Closed
cpossel opened this issue Sep 6, 2022 · 7 comments
Closed

Default bounds for UCC ansatz circuits #818

cpossel opened this issue Sep 6, 2022 · 7 comments
Assignees

Comments

@cpossel
Copy link
Contributor

cpossel commented Sep 6, 2022

What is the expected enhancement?

Bounds are required for many optimizers (all of the package scikit-quant as can be read on page 9 of their documentation and possibly some of the other optimizers).

Most of the ansatzes located in qiskit terra (EfficientSU2, ExcitationPreservation, ...) have default bounds for the parameters (-pi to pi). See e.g. method parameter_bounds of EfficientSU2.

The ansatzes in qiskit nature (UCC, ...) don't have these default bounds implemented. Thus, those VQE calculations fail as pointed out in this bug report.

So, I propose to add the parameter_bounds method also to the ansatzes is qiskit nature.

I'll gladly help adding the code.

@mrossinek
Copy link
Member

The problem here is that the UCC ansatz has no concept of parameter bounds, since the parameters in this ansatz can take arbitrary values. I do not know, whether returning float("NaN") rather than None would improve things, but I do not see a reasonable solution here.

Maybe we could expose the parameter bounds to the user such that they can tweak things to at least make the algorithm runnable. But I would be hesitant as this would be a very fragile approach which could potentially lead to very inaccurate results if misused by the end-user.

@cpossel
Copy link
Contributor Author

cpossel commented Sep 7, 2022

I see the point and already feared that it would turn out like that...
Returning NaN instead of None would improve nothing since the underlying algorithms initialize the bounds with None either way when they don't find any.

As far as I have insight into the UCC ansatz, the parameters are only part of rotation gates. So, one could use the knowledge that a rotation gate can only rotate around 2pi (footnotemark) leading to effective bounds of (-pi, pi). With that one could e.g. for a Ry(2*theta) gate in the circuit set the bounds to (-pi/2, pi/2) i.e. streching the parameter bounds so that they fit into the (-pi, pi) scheme.
Of course this would be a bit more coding work as the bound setting method would need to access the circuit details but in principle I can't see anything speaking against it. Or do I miss anything?

footnote: Of course the rotation gates can take arbitrary values but one can always map them back to the (-pi, pi) interval without loss of generality.

Exposing the bounds to the end-user is in my opinion not a good approach. For this actually the functionality of specifying an initial point should be sufficient (and is less fragile).
If above proposed approach doesn't work a workaround would be to set (-inf, inf) as interval (or any high number close to inf).

@mrossinek
Copy link
Member

Sorry for the long delay on getting back to you. A partial reason for the delay is that I simply do not have an ideal answer to this problem. The scaling of the parameter bounds to fit within the (-pi, pi) interval does not seem like a scalable approach to me as I am not sure about the implications on the Terra-side of the code.

If float("NaN") is not a solution, I assume using float("inf") would improve things in the sense that the optimizers will become runnable (but not necessarily usable since bounds are at least defined although not necessarily with useful values).

If you want to open a PR which adds these bounds to the classes in the qiskit_nature.second_q.circuit module we could see how things behave and go from there 👍

@cpossel
Copy link
Contributor Author

cpossel commented Nov 15, 2022

The question now is whether float("inf") should be the default instead of None (similarly to EfficientSU2 just hard-coded in UCC) or to dynamically set it to inf if the optimizer requires it. This would be possible within vqe.py or rather in the function validate_bounds called by VQE (located in validate_bounds.py). There the VQE can actually check whether the optimizer needs bounds and then could set them if required. This would have the advantage that these arbitrary bounds would be inserted only when needed (allowing for unbounded optimizers to work properly).

As soon as this question is clarified, I'll gladly add some code to implement it and open a PR (though in the latter case it would be in qiskit terra code).

@mrossinek
Copy link
Member

I will raise this idea to the people for involved with the affected components of Qiskit Terra and get back to you 👍

@woodsp-ibm
Copy link
Member

See Qiskit/qiskit#9718 which I created recently that more broadly talks about an improvement to bounds

@mrossinek
Copy link
Member

The general direction of the broader issue linked by Steve above was that bounds should not be coupled to the ansatz.
Instead, we seemed to converge to treat them analogously to the initial_point argument of variational algorithms.

Thus, I am closing this issue here.

This change has not been implemented yet but I believe that further discussion (and potentially PRs) are welcome at qiskit-community/qiskit-algorithms#57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants