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

Fix QAOA #5622

Merged
merged 5 commits into from
Jan 15, 2021
Merged

Fix QAOA #5622

merged 5 commits into from
Jan 15, 2021

Conversation

t-imamichi
Copy link
Member

@t-imamichi t-imamichi commented Jan 13, 2021

Summary

Fix QAOA of opflow.

Details and comments

  • Allow users to change the number of qubits of QAOA
    • Change the order of check and varform initialization
  • Track appropriate parameters associated with QAOA var form
    • Add a check of number of qubits

@t-imamichi t-imamichi marked this pull request as draft January 13, 2021 15:11
@t-imamichi
Copy link
Member Author

t-imamichi commented Jan 13, 2021

I found that this change breaks some unit tests of chemistry, e.g., test/chemistry/test_uccsd_hartree_fock.py
I need to check the details.

Update: I fixed this problem. See my comment below.

@t-imamichi t-imamichi marked this pull request as ready for review January 13, 2021 16:19
@t-imamichi t-imamichi requested a review from woodsp-ibm January 13, 2021 16:19
@t-imamichi
Copy link
Member Author

t-imamichi commented Jan 13, 2021

I found out the root cause. QAOA._check_operator generates new parameters with same label, which have different hash value. I fix it by checking number of qubits as VQE._check_operator_varform does. We need the check in both VQE and QAOA because they are both invoked.

@woodsp-ibm
Copy link
Member

Ok. At first sight it looks like the order of the check_operator and creation of var form were just swapped with the added qubit check that you would think was for efficiency to prevent building out the var form again, and if you did it would not be an issue. The check though is in fact needed because of this parameter sorting.

With the code change on the move from Aqua, which removed the operator from constructor and the setter, meaning it can only be passed on the compute_min_eigval, I think going forwards there is room to improve refactor/improve the logic around this checking/building etc. Especially if the parameters end up with a defined ordering #5614

@manoelmarques manoelmarques merged commit d6ba630 into Qiskit:master Jan 15, 2021
@t-imamichi t-imamichi deleted the fix-qaoa branch January 15, 2021 04:43
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
* Fix QAOA

* revert unnecessary change
ElePT pushed a commit to ElePT/qiskit-algorithms-test that referenced this pull request Jul 17, 2023
* Fix QAOA

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

Successfully merging this pull request may close these issues.

3 participants