Add feasibility checks to optimizers/converters #1199

KahanMajmudar · 2020-08-13T16:07:41Z

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 · 2020-08-13T16:07:47Z

All committers have signed the CLA.

t-imamichi · 2020-08-13T16:13:55Z

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 · 2020-08-13T16:40:21Z

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 · 2020-08-13T16:55:48Z

@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

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

qiskit/optimization/algorithms/cplex_optimizer.py

adekusar-drl · 2020-08-13T19:36:58Z

qiskit/optimization/problems/quadratic_program.py

+        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'))
+


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

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.

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.

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

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.

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

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

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

adekusar-drl · 2020-08-13T20:00:56Z

@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.

…ssue1134

into issue1134

t-imamichi · 2020-08-18T12:24:12Z

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.

…ssue1134

into issue1134

adekusar-drl · 2020-09-02T20:12:12Z

qiskit/optimization/algorithms/cplex_optimizer.py

                                    fval=sol.get_objective_value(),
                                    variables=problem.variables,
-                                    raw_results=sol)
+                                    raw_results=problem.variables,


Why is raw_results=problem.variables ? originally it was raw_results=sol.

adekusar-drl · 2020-09-02T20:19:45Z

qiskit/optimization/algorithms/optimization_algorithm.py

-    FAILURE = 1
-    """the optimization algorithm ended in a failure."""
+        Raises:
+            None


If no exception is raised, section "Raises" should be omitted.

adekusar-drl · 2020-09-02T20:22:32Z

qiskit/optimization/algorithms/minimum_eigen_optimizer.py

                 samples: List[Tuple[str, float, float]],
-                 min_eigen_solver_result: Optional[MinimumEigensolverResult] = None) -> None:
+                 min_eigen_solver_result: Optional[MinimumEigensolverResult] = None,
+                 status: OptimizationResultStatus = OptimizationResultStatus.SUCCESS) -> None:


Please make status parameter required.

I do not think there is something like Required inside the typing provided by python
https://github.com/python/typing/blob/5d2f3b1992b8b66d41fe001df148693b45d30e31/src/typing.py#L23-L98

I meant, delete default assignment to status, this will cause the parameter to be required, you have to pass a value for this parameter.

Correct, but I do need to define a default value for it. Should I go with None?

No, no default value, that's the thing.

Then it throws an error: default value must be specified here Python(parser-16)

Non default(required) parameters go first. So, move it up on the list of parameters, we still can do this.

adekusar-drl · 2020-09-02T20:24:08Z

qiskit/optimization/algorithms/recursive_minimum_eigen_optimizer.py

                 replacements: Dict[str, Tuple[str, int]],
-                 history: Tuple[List[MinimumEigenOptimizationResult], OptimizationResult]) -> None:
+                 history: Tuple[List[MinimumEigenOptimizationResult], OptimizationResult],
+                 status: OptimizationResultStatus = OptimizationResultStatus.SUCCESS) -> None:


Same here, status is required.

adekusar-drl · 2020-09-02T20:24:54Z

qiskit/optimization/algorithms/slsqp_optimizer.py

                 fx: Optional[np.ndarray] = None, its: Optional[int] = None,
-                 imode: Optional[int] = None, smode: Optional[str] = None) -> None:
+                 imode: Optional[int] = None, smode: Optional[str] = None,
+                 status: OptimizationResultStatus = OptimizationResultStatus.SUCCESS) -> None:


Same here, please make status required.

adekusar-drl · 2020-09-02T20:28:01Z

qiskit/optimization/algorithms/slsqp_optimizer.py

+        # check for feasibility
+        status = OptimizationResultStatus.SUCCESS if problem.is_feasible(result.x) \
+            else OptimizationResultStatus.INFEASIBLE


These lines are not required any more

adekusar-drl · 2020-09-02T20:33:41Z

qiskit/optimization/problems/quadratic_program.py

+        # create a dict containing only unsatisfied variable(s)/constraint(s)
+        final_dict = {k: v for k, v in {**satisfied_variables, **satisfied_constraints}
+                      .items() if not v}
+
+        # debug
+        logger.debug("Violations: %s", final_dict)


final_dict is created for debug purposes only. Please create it only when debug is enabled or remove this.

into issue1134

qiskit/optimization/problems/quadratic_program.py

adekusar-drl · 2020-09-09T19:55:09Z

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

t-imamichi · 2020-09-10T06:30:39Z

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

into issue1134

releasenotes/notes/feasibility-check-b99605f771e745b7.yaml

qiskit/optimization/problems/quadratic_program.py

qiskit/optimization/algorithms/optimization_algorithm.py

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

…ssue1134

KahanMajmudar · 2020-09-11T08:34:12Z

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 · 2020-09-11T10:01:48Z

Congrats! Enjoy Qiskit hacking.

@adekusar-drl

* 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 <[email protected]> * Update releasenotes/notes/feasibility-check-b99605f771e745b7.yaml Co-authored-by: Steve Wood <[email protected]> * code review Co-authored-by: Manoel Marques <[email protected]> Co-authored-by: Takashi Imamichi <[email protected]> Co-authored-by: Anton Dekusar <[email protected]> Co-authored-by: Anton Dekusar <[email protected]> Co-authored-by: Steve Wood <[email protected]>

@adekusar-drl

…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 <[email protected]> * Update releasenotes/notes/feasibility-check-b99605f771e745b7.yaml Co-authored-by: Steve Wood <[email protected]> * code review Co-authored-by: Manoel Marques <[email protected]> Co-authored-by: Takashi Imamichi <[email protected]> Co-authored-by: Anton Dekusar <[email protected]> Co-authored-by: Anton Dekusar <[email protected]> Co-authored-by: Steve Wood <[email protected]>

Draft for issue#1134

f283cd2

KahanMajmudar changed the title ~~Draft for issue#1134~~ [WIP] Draft for issue#1134 Aug 13, 2020

t-imamichi requested review from t-imamichi and adekusar-drl August 13, 2020 16:14

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

Resolved merge conflict by incorporating new changes

ab2c833

KahanMajmudar and others added 2 commits August 13, 2020 22:27

Merge branch 'master' into issue1134

97bddff

resolved issue in quadratic_problem.py

96e3622

adekusar-drl linked an issue Aug 13, 2020 that may be closed by this pull request

Add feasibility checks to optimizers/converters #1134

Closed

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

adekusar-drl reviewed Aug 13, 2020

View reviewed changes

adekusar-drl added Optimization type: enhancement labels Aug 13, 2020

KahanMajmudar and others added 5 commits August 14, 2020 19:23

incorporated changes as suggested by @adekusar-drl

1eab962

Merge branch 'master' into issue1134

a5cab4a

Merge branch 'master' into issue1134

408a54a

Merge branch 'master' of https://github.com/Qiskit/qiskit-aqua into i…

558228a

…ssue1134

Merge branch 'issue1134' of https://github.com/KahanMajmudar/qiskit-aqua

56a29c5

into issue1134

t-imamichi and others added 6 commits August 18, 2020 21:24

Merge branch 'master' into issue1134

bd9ab0f

Merge branch 'master' into issue1134

4058d56

Merge branch 'master' of https://github.com/Qiskit/qiskit-aqua into i…

c5b41cf

…ssue1134

Merge branch 'issue1134' of https://github.com/KahanMajmudar/qiskit-aqua

ce6008f

into issue1134

fix linting issues

791838e

Merge branch 'master' into issue1134

c4f2f0f

adekusar-drl reviewed Sep 2, 2020

View reviewed changes

manoelmarques and others added 5 commits September 2, 2020 18:21

Merge branch 'master' into issue1134

4425e97

made suggested changes by @adekusar-drl

5ec3a6d

Merge branch 'issue1134' of https://github.com/KahanMajmudar/qiskit-aqua

038009b

into issue1134

Merge branch 'master' into issue1134

3ad20b8

Merge branch 'master' into issue1134

8d6f784

t-imamichi reviewed Sep 9, 2020

View reviewed changes

qiskit/optimization/problems/quadratic_program.py Outdated Show resolved Hide resolved

t-imamichi reviewed Sep 9, 2020

View reviewed changes

qiskit/optimization/problems/quadratic_program.py Outdated Show resolved Hide resolved

t-imamichi reviewed Sep 9, 2020

View reviewed changes

qiskit/optimization/problems/quadratic_program.py Outdated Show resolved Hide resolved

adekusar-drl and others added 2 commits September 9, 2020 19:55

some cleaning

274f9bc

Merge branch 'master' into issue1134

fbaecaa

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

adekusar-drl added 3 commits September 10, 2020 10:12

added reno

d58f934

Merge branch 'issue1134' of https://github.com/KahanMajmudar/qiskit-aqua

e73c091

into issue1134

fixed reno

818cadd

KahanMajmudar requested a review from levbishop as a code owner September 10, 2020 10:05

woodsp-ibm reviewed Sep 10, 2020

View reviewed changes

adekusar-drl and others added 5 commits September 10, 2020 22:50

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

8a0a10e

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

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

286d68d

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

Merge branch 'master' of https://github.com/Qiskit/qiskit-aqua into i…

718ad72

…ssue1134

code review

cb56d8c

Merge branch 'master' into issue1134

dfe4537

woodsp-ibm approved these changes Sep 10, 2020

View reviewed changes

adekusar-drl merged commit 99394e5 into qiskit-community:master Sep 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feasibility checks to optimizers/converters #1199

Add feasibility checks to optimizers/converters #1199

KahanMajmudar commented Aug 13, 2020

CLAassistant commented Aug 13, 2020 •

edited

Loading

t-imamichi commented Aug 13, 2020

KahanMajmudar commented Aug 13, 2020

KahanMajmudar commented Aug 13, 2020

adekusar-drl left a comment

adekusar-drl Aug 13, 2020

KahanMajmudar Aug 14, 2020

adekusar-drl Aug 14, 2020

adekusar-drl Aug 14, 2020

KahanMajmudar Aug 14, 2020

KahanMajmudar Aug 14, 2020

adekusar-drl Aug 14, 2020

KahanMajmudar Aug 14, 2020

adekusar-drl commented Aug 13, 2020

t-imamichi commented Aug 18, 2020

adekusar-drl Sep 2, 2020

adekusar-drl Sep 2, 2020

adekusar-drl Sep 2, 2020

KahanMajmudar Sep 3, 2020

adekusar-drl Sep 3, 2020 •

edited

Loading

KahanMajmudar Sep 3, 2020

adekusar-drl Sep 3, 2020

KahanMajmudar Sep 3, 2020

adekusar-drl Sep 3, 2020

adekusar-drl Sep 2, 2020

adekusar-drl Sep 2, 2020

adekusar-drl Sep 2, 2020

adekusar-drl Sep 2, 2020

adekusar-drl commented Sep 9, 2020

t-imamichi commented Sep 10, 2020

KahanMajmudar commented Sep 11, 2020

t-imamichi commented Sep 11, 2020

Add feasibility checks to optimizers/converters #1199

Add feasibility checks to optimizers/converters #1199

Conversation

KahanMajmudar commented Aug 13, 2020

Summary

Details and comments

CLAassistant commented Aug 13, 2020 • edited Loading

t-imamichi commented Aug 13, 2020

KahanMajmudar commented Aug 13, 2020

KahanMajmudar commented Aug 13, 2020

adekusar-drl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adekusar-drl commented Aug 13, 2020

t-imamichi commented Aug 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adekusar-drl Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adekusar-drl commented Sep 9, 2020

t-imamichi commented Sep 10, 2020

KahanMajmudar commented Sep 11, 2020

t-imamichi commented Sep 11, 2020

CLAassistant commented Aug 13, 2020 •

edited

Loading

adekusar-drl Sep 3, 2020 •

edited

Loading