Skip to content
This repository has been archived by the owner on Dec 7, 2021. It is now read-only.

Further enhancements in ADMMOptimizer #1126

Merged
merged 12 commits into from
Jul 24, 2020

Conversation

adekusar-drl
Copy link
Contributor

@adekusar-drl adekusar-drl commented Jul 21, 2020

Summary

Removed soft depenedency on CplexOptimizer, now MinEigenOptimizer and SlsqpOptimizer are used by default.
Added repr support to ADMMParameters.
Added getter/setter for the parameters to ADMMOptimizer.

@adekusar-drl adekusar-drl changed the title Convenient methods in ADMMOptimizer [WIP] Convenient methods in ADMMOptimizer Jul 21, 2020
stefan-woerner
stefan-woerner previously approved these changes Jul 21, 2020
@adekusar-drl adekusar-drl changed the title [WIP] Convenient methods in ADMMOptimizer Further enhancements in ADMMOptimizer Jul 21, 2020
woodsp-ibm
woodsp-ibm previously approved these changes Jul 21, 2020
Copy link
Member

@woodsp-ibm woodsp-ibm left a comment

Choose a reason for hiding this comment

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

After these changes there is no test then of ADMMOptimizer with the CPLEXOptimizer even specifying it explicitly. Maybe the latter does not need to be included in the testing here in what is really more of an integration test since the same function of ADMM is tested using other optimizers.

@woodsp-ibm
Copy link
Member

One other comment - maybe the ADMMOptimizer docstring should state the default qubo and continuous optimizers that are selected internally - or at least say if left as default then a suitable one will chosen internally even if we don't care to explicitly name which one.

@adekusar-drl
Copy link
Contributor Author

@woodsp-ibm, since we made dependency on Cplex optional and tests with Cplex are not run by the build system (well, they are skipped, correct me if I'm wrong here) I think it is better to use other optimizers internally, especially once we have an SLSQP wrapper. That's why I removed Cplex in the implementation and in the tests as well.

I will update the docstring tomorrow, it is a good point to mention how internal optimizers are chosen if they are not specified.

@woodsp-ibm
Copy link
Member

I think tests with Cplex are run under Python 3.7 and the reason for the skip is that Cplex is not yet supported for 3.8 under which we also test in the build.

@adekusar-drl
Copy link
Contributor Author

Yes, you're right, Cplex does not support python 3.8. Anyway, there's a test where a non-default (Cobyla) continuous optimizer is used, just in case.
Updated the docstring, please review/approve.

@woodsp-ibm
Copy link
Member

I think for this ":class:NumPyMinimumEigensolver" to resolve as a clickable link it would have to be fully qualified ":class:~qiskit.aqua.algorithms.NumPyMinimumEigensolver" The ~ means it will show just the last part so it would look like you intended. (The other ref there should work as is since its in the currentmodule.) BTW you can check check the links in html output by cd to docs folder and there 'make html' which will build out the html and you find the index.html in a subfolder and you can navigate to this class from there

@adekusar-drl
Copy link
Contributor Author

At least in my setup a link ":class:NumPyMinimumEigensolver" is clickable. I have verified this before committing by building html documentation. I guess if you have an import, then you don't need a fully qualified class name.
Or you think it is better to have a full name?
P.S. build fails due other reasons, a fews PR in the same situation.

@woodsp-ibm
Copy link
Member

Ok, with the import - I guess I should have thought. I have had to fix up a few of these where another class in another module has been referenced and its not been working. Its fine as-is then, no need to make it fully qualified.

@adekusar-drl adekusar-drl merged commit 0464a4d into qiskit-community:master Jul 24, 2020
manoelmarques added a commit to manoelmarques/qiskit-aqua that referenced this pull request Jul 28, 2020
* ADMMParameters repr

* ADMM properties, tests

* linting

* fix tests

* removed default dependency on cplex

* import fix for pylint

* added reno file for ADMM

* updated docstring

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Manoel Marques <manoel@us.ibm.com>
@adekusar-drl adekusar-drl deleted the admm_conv branch August 5, 2020 11:50
pbark pushed a commit to pbark/qiskit-aqua that referenced this pull request Sep 16, 2020
* ADMMParameters repr

* ADMM properties, tests

* linting

* fix tests

* removed default dependency on cplex

* import fix for pylint

* added reno file for ADMM

* updated docstring

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Manoel Marques <manoel@us.ibm.com>
manoelmarques added a commit to qiskit-community/qiskit-optimization that referenced this pull request Jan 14, 2021
)

* ADMMParameters repr

* ADMM properties, tests

* linting

* fix tests

* removed default dependency on cplex

* import fix for pylint

* added reno file for ADMM

* updated docstring

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Manoel Marques <manoel@us.ibm.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants