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

Add feasibility checks to optimizers/converters #1199

Merged
merged 45 commits into from
Sep 11, 2020

Conversation

KahanMajmudar
Copy link
Contributor

Summary

draft of #1134
Added is_feasible() function to check whether the returned result of the optimizers and to check whether they are feasible or not

Details and comments

Please check for the same and provide valuable feedback. Thank you 😃

@CLAassistant
Copy link

CLAassistant commented Aug 13, 2020

CLA assistant check
All committers have signed the CLA.

@KahanMajmudar KahanMajmudar changed the title Draft for issue#1134 [WIP] Draft for issue#1134 Aug 13, 2020
@t-imamichi
Copy link
Contributor

Please change the title to be more informative to denote what this PR adds and merge master to your branch. There are some conflicts.

@KahanMajmudar KahanMajmudar changed the title [WIP] Draft for issue#1134 [WIP] Draft for issue#1134. Adds a is_feasible() function to check for feasibility of a solution returned by the solve method Aug 13, 2020
@KahanMajmudar
Copy link
Contributor Author

Please change the title to be more informative to denote what this PR adds and merge master to your branch. There are some conflicts.

Done and Done. 😄

@KahanMajmudar
Copy link
Contributor Author

@t-imamichi I am confused by this PR. It already incorporates the changes of is_feasible(x) function in optimizers. So after this(draft added by me) PR addes the function, does your PR makes the necessary changes or do I have to work on adding the function to every optimzier by following your PR.

@adekusar-drl adekusar-drl linked an issue Aug 13, 2020 that may be closed by this pull request
@adekusar-drl adekusar-drl changed the title [WIP] Draft for issue#1134. Adds a is_feasible() function to check for feasibility of a solution returned by the solve method [WIP] Add feasibility checks to optimizers/converters Aug 13, 2020
Copy link
Contributor

@adekusar-drl adekusar-drl left a comment

Choose a reason for hiding this comment

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

Keeping going! All good as an intermediate version!
Please think of adding unit tests. Also, pay attention at the pipeline output, stuff like pylint, code style, spelling, etc.

qiskit/optimization/algorithms/admm_optimizer.py Outdated Show resolved Hide resolved
qiskit/optimization/algorithms/admm_optimizer.py Outdated Show resolved Hide resolved
qiskit/optimization/algorithms/cplex_optimizer.py Outdated Show resolved Hide resolved
Comment on lines 1133 to 1138
if(detailed == True):

for i in range(len(satisfied_variables)):
(print(f'{self.get_variable(i)._name} is within the bounds') if satisfied_variables[i]
else logger.warning(f'{self.get_variable(i)._name} is outside the bounds'))

Copy link
Contributor

Choose a reason for hiding this comment

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

The initial idea was to return a list of violations, not just printing them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial idea was to return a list of violations, not just printing them.

This is for debug purposes only. I was not sure about how the return value was required. I will change it into a list.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good question how to return them back. May be we should postpone this feature, I mean returning a list of violations and focus on the boolean result.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd expect to see a list of something, but what is float here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So currently I will print the violations and just return the bool result. One way would be to change the return type from ->bool to ->Tuple[bool, List[float]] and return empty list whenever detailed = False.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The list can contain float values as well. Like x = [1.0, 2.0, 1.0, 1.99999982]

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but what do they mean, these float values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry I didn't get your question. I would try to explain from what I understood.
So if we define a quadratic problem with variables x, y and z, then we get results.x = [1.0, 2.0, 1.0] would mean x = 1.0, y = 2.0 and z = 1.0.

qiskit/optimization/problems/quadratic_program.py Outdated Show resolved Hide resolved
qiskit/optimization/problems/quadratic_program.py Outdated Show resolved Hide resolved
@adekusar-drl
Copy link
Contributor

@t-imamichi I am confused by this PR. It already incorporates the changes of is_feasible(x) function in optimizers. So after this(draft added by me) PR addes the function, does your PR makes the necessary changes or do I have to work on adding the function to every optimzier by following your PR.

As I understood from the description of that PR, you don't have to follow that PR. Main purpose of #1196 is to change the interpret method in the converters. So it should may use of this PR.

@t-imamichi
Copy link
Contributor

Yes. You can ignore is_feasible(x) in #1196. It is a tentative implementation of feasibility check to develop my PoC. Your implementation will be more intuitive and efficient.

@adekusar-drl
Copy link
Contributor

@woodsp-ibm, @t-imamichi please take a look, should be fine now.

@t-imamichi t-imamichi added the Changelog: API Change Include in the Changed section of the changelog label Sep 10, 2020
@t-imamichi
Copy link
Contributor

Thank you! It looks good. We need just a reno text to explain the new APIs.

@adekusar-drl adekusar-drl merged commit 99394e5 into qiskit-community:master Sep 11, 2020
@KahanMajmudar
Copy link
Contributor Author

Thank you everyone 😄. I am glad that my first ever PR has been accepted and merged successfully. Thank you @adekusar-drl and @t-imamichi for giving me the opportunity to work on this 🎉.

@t-imamichi
Copy link
Contributor

Congrats! Enjoy Qiskit hacking.

pbark pushed a commit to pbark/qiskit-aqua that referenced this pull request Sep 16, 2020
* Draft for issue#1134

* resolved issue in quadratic_problem.py

* incorporated changes as suggested by @adekusar-drl

* fix linting issues

* added is_feasible method in all optimizers, added changes suggested by @ woodsp-ibm

* removed cast import

* made changes as per points 1 to 3

* made suggested changes by @adekusar-drl

* some cleaning

* added reno

* fixed reno

* Update releasenotes/notes/feasibility-check-b99605f771e745b7.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Update releasenotes/notes/feasibility-check-b99605f771e745b7.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* code review

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
manoelmarques added a commit to qiskit-community/qiskit-optimization that referenced this pull request Jan 14, 2021
…kit-aqua#1199)

* Draft for issue#1134

* resolved issue in quadratic_problem.py

* incorporated changes as suggested by @adekusar-drl

* fix linting issues

* added is_feasible method in all optimizers, added changes suggested by @ woodsp-ibm

* removed cast import

* made changes as per points 1 to 3

* made suggested changes by @adekusar-drl

* some cleaning

* added reno

* fixed reno

* Update releasenotes/notes/feasibility-check-b99605f771e745b7.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* Update releasenotes/notes/feasibility-check-b99605f771e745b7.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

* code review

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Co-authored-by: Anton Dekusar <adekusar@ie.ibm.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog: API Change Include in the Changed section of the changelog Optimization type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add feasibility checks to optimizers/converters
6 participants