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

Update AdaptVQE to new, stateless VQE #222

Merged
merged 3 commits into from
Jun 18, 2021

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Jun 3, 2021

Summary

Fix the Adapt VQE after Qiskit/qiskit#6418 is merged.

Note: This can only be merged after the above PR in Terra is merged.

Details and comments

AdaptVQE previously relied on an internal private method which has been deleted without deprecation (since it was private).

which previously used the (now deleted) internal method _evaluate_energy of the VQE
@woodsp-ibm
Copy link
Member

woodsp-ibm commented Jun 3, 2021

Since this change requires what is in Terra main, i,e, version 0.18.0 when its released, I think we should update the requirements.txt file to change this qiskit-terra>=0.17.0 to qiskit-terra>=0.18.0. This way when we backport it, it will have the right requirements on Terra.

Update: I had asked the same of #210 which has now been merged. So we will have to take care when backporting this around the Terra version in stable.

@mrossinek mrossinek added priority: critical and removed on hold Can not fix yet labels Jun 17, 2021
@mrossinek
Copy link
Member

The CI will continue to fail until this is merged because 5 hours ago the related PR in Terra got merged 👍

@woodsp-ibm
Copy link
Member

@Cryoris This is failing with some issue in vqe from the BopesSampler tutorial. Can you please take a look.

@manoelmarques
Copy link
Contributor

manoelmarques commented Jun 17, 2021

@Cryoris I created PR to fix this: Qiskit/qiskit#6599

The error in the tutorial is due to a Terra decorator deprecate_function that is not wrapping properties correctly and returns a decorator wrapper instead of the property. If I remove the deprecate_decorator from:

@deprecate_function(
        """
The VQE.optimal_params property is deprecated as of Qiskit Terra 0.18.0
and will be removed no sooner than 3 months after the releasedate.
This information is part of the returned result object and can be
queried as VQEResult.optimal_point."""
    )
    @property
    def optimal_params(self) -> np.ndarray:

It works. If I keep the decorator, instead of returning an array, the property returns:

<bound method deprecate_function.<locals>.decorator.<locals>.wrapper of <qiskit.algorithms.minimum_eigen_solvers.vqe.VQE object at 0x7ff8a2980b50>>

If I change the order putting the property above the deprecate_function, it works:

@property
@deprecate_function(
        """
The VQE.optimal_params property is deprecated as of Qiskit Terra 0.18.0
and will be removed no sooner than 3 months after the releasedate.
This information is part of the returned result object and can be
queried as VQEResult.optimal_point."""
    )
    def optimal_params(self) -> np.ndarray:

So, all the properties that use this decorator should have it after the property decorator. A little tricky.
This was added by PR Stateless VQE: Qiskit/qiskit#6418

@woodsp-ibm
Copy link
Member

I guess the BopesSampler should be fixed to avoid the deprecated method/property that it tripped over as described above. Since this is holding up CI maybe thats best done via another issue/PR.

Copy link
Member

@mrossinek mrossinek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woodsp-ibm what issue did you see? The CI is passing, right?

@mrossinek
Copy link
Member

@woodsp-ibm what issue did you see? The CI is passing, right?

Ah I see. Because this fix is merged now: Qiskit/qiskit#6599

Copy link
Contributor

@pbark pbark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Cryoris
Copy link
Contributor Author

Cryoris commented Jun 18, 2021

I think we might still need to add the requirement Terra >= 0.18 before merging 🙂

@mrossinek
Copy link
Member

I think we might still need to add the requirement Terra >= 0.18 before merging

This already happened or do we need another requirement? See: https://github.com/Qiskit/qiskit-nature/blob/main/requirements.txt

@Cryoris Cryoris merged commit a1300f5 into qiskit-community:main Jun 18, 2021
@Cryoris Cryoris deleted the fix-adapt-vqe branch June 18, 2021 12:08
@woodsp-ibm
Copy link
Member

woodsp-ibm commented Jun 18, 2021

I had also asked for the requirement to be updated in another PR which had a dependence on newer Terra see above comment #222 (comment). Since we need to backport this as a bug fix release for the upcoming Terra to assure nature still works with the latest Terra we will have to remember to change the requirements.txt file in stable when doing the stable backport for the release - @manoelmarques fyi

manoelmarques pushed a commit to manoelmarques/qiskit-nature that referenced this pull request Jul 7, 2021
* fix AdaptVQE

which previously used the (now deleted) internal method _evaluate_energy of the VQE

* add reno

Co-authored-by: Steve Wood <[email protected]>
manoelmarques pushed a commit to manoelmarques/qiskit-nature that referenced this pull request Jul 8, 2021
* fix AdaptVQE

which previously used the (now deleted) internal method _evaluate_energy of the VQE

* add reno

Co-authored-by: Steve Wood <[email protected]>
manoelmarques added a commit that referenced this pull request Jul 12, 2021
* Prepare 0.1.4 release

* Deprecated Location: Improve Windows compatibility (pathlib) for psi4 (#266)

* Update AdaptVQE to new, stateless VQE (#222)

* fix AdaptVQE

which previously used the (now deleted) internal method _evaluate_energy of the VQE

* add reno

Co-authored-by: Steve Wood <[email protected]>

* Bump Terra min. requirement

Co-authored-by: Julien Gacon <[email protected]>
Co-authored-by: Steve Wood <[email protected]>
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request May 25, 2023
* fix AdaptVQE

which previously used the (now deleted) internal method _evaluate_energy of the VQE

* add reno

Co-authored-by: Steve Wood <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants