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

Change QuadraticProgramConverter.interpret to convert x directly #1196

Merged
merged 28 commits into from
Nov 25, 2020
Merged

Change QuadraticProgramConverter.interpret to convert x directly #1196

merged 28 commits into from
Nov 25, 2020

Conversation

t-imamichi
Copy link
Contributor

@t-imamichi t-imamichi commented Aug 13, 2020 •
edited
Loading

Summary

QuadraticProgramConverter.interpret currently converts OptimizationResult to OptimizationResult.
The main functionality of interpret is the evaluation of the values of variables.
The main idea of this pullreq is to simplify interpret to deal with only x.
It also extends OptimizationAlgorithm._interpret to generate a subclass of OptimizationResult to set the variables and status automatically.

It removes deprecated methods QuadraticProgramConverter.{encode,decode} and deprecated classes QuadraticProgramToIsing and IsingToQuadraticProgram.

Details and comments

This was referenced Aug 13, 2020
Merged
Closed
@KahanMajmudar KahanMajmudar mentioned this pull request Aug 13, 2020
Merged
@adekusar-drl
Copy link
Contributor

@t-imamichi Thanks, looks good. Seems like this change did not affect the converters seriously and we may replace a pattern where we create two optimization results (base and specific) with a single result.
As I understood you suggest to keep just interpret(x: ndarray) -> ndarray only, do you?

@t-imamichi
Copy link
Contributor Author

Thank you. Yes, you are right. I have to remove the deprecated encode and decode in this PR. I don't think it's a good idea to keep two implementations of decode and interpret.
I notice that no optimizer uses status OptimizationResultStatus.FAILURE. If it is not necessary, we can simplify it too. What do you think?

@a-matsuo
Copy link
Contributor

Shouldn't we wait at least 3 months before removing deprecated 'encode' and 'decode' as mentioned in #1178?

@t-imamichi
Copy link
Contributor Author

t-imamichi commented Aug 19, 2020 •
edited
Loading

That's why this PR is draft still.

@t-imamichi
Copy link
Contributor Author

t-imamichi commented Aug 19, 2020 •
edited
Loading

Currently, no algorithms takes care of the status of algorithm. So, we cannot detect that the algorithms terminates successfully or not. For example, Cplex has cplex.solution.get_status and fmin_slsqp returns imode. But, we don't check them now.
https://www.ibm.com/support/knowledgecenter/SSSA5P_12.10.0/ilog.odms.cplex.help/refpythoncplex/html/cplex._internal._subinterfaces.SolutionInterface-class.html#get_status
https://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.fmin_slsqp.html
How about check them and set it as OptimizationResultStatus?

My current impl in this pullreq sets SUCCESS if a feasible solution is obtained. So, even if the solution is far from optimal, it can be marked as SUCCESS if feasible.

@woodsp-ibm
Copy link
Member

How does this and #1199 come together - it seems there is overlap. I made some comments in #1199

@t-imamichi
Copy link
Contributor Author

Thank you for your comments. This will come after #1199 because this requires is_feasible. The current impl of is_feasible in this PR is just a tentative one. Because it removes deprecated methods encode and decode and change API of interpret, I would like to discuss what is a good design of interpret.

t-imamichi and others added 4 commits August 21, 2020 12:24
@t-imamichi
Merge branch 'master' into interpret
5de37c3
@manoelmarques
Merge branch 'master' into interpret
47f853f
@t-imamichi
Merge branch 'master' into interpret
0d1d279 
# Conflicts:
#	qiskit/optimization/algorithms/admm_optimizer.py
#	qiskit/optimization/algorithms/cplex_optimizer.py
#	qiskit/optimization/algorithms/grover_optimizer.py
#	qiskit/optimization/algorithms/minimum_eigen_optimizer.py
#	qiskit/optimization/algorithms/multistart_optimizer.py
#	qiskit/optimization/algorithms/recursive_minimum_eigen_optimizer.py
#	qiskit/optimization/algorithms/slsqp_optimizer.py
#	qiskit/optimization/converters/inequality_to_equality.py
#	qiskit/optimization/converters/integer_to_binary.py
#	qiskit/optimization/converters/linear_equality_to_penalty.py
#	qiskit/optimization/converters/quadratic_program_converter.py
#	qiskit/optimization/converters/quadratic_program_to_qubo.py
#	qiskit/optimization/problems/quadratic_program.py
#	test/optimization/test_converters.py
@t-imamichi
change interpret and remove deprecated methods and classes
d9d7b71
@t-imamichi t-imamichi marked this pull request as ready for review November 19, 2020 12:50
adekusar-drl
adekusar-drl previously approved these changes Nov 20, 2020
View reviewed changes
@t-imamichi t-imamichi added Changelog: Deprecation Include in Deprecated section of changelog Changelog: API Change Include in the Changed section of the changelog labels Nov 24, 2020
stefan-woerner
stefan-woerner previously approved these changes Nov 24, 2020
View reviewed changes
a-matsuo
a-matsuo previously approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@a-matsuo a-matsuo left a comment

Choose a reason for hiding this comment

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

LGTM!

@t-imamichi t-imamichi merged commit 6e9a58b into qiskit-community:master Nov 25, 2020
@t-imamichi t-imamichi deleted the interpret branch November 25, 2020 14:51
manoelmarques added a commit to qiskit-community/qiskit-optimization that referenced this pull request Jan 14, 2021
@t-imamichi @manoelmarques @adekusar-drl
Change QuadraticProgramConverter.interpret to convert x directly (q…
14c33a2 
…iskit-community/qiskit-aqua#1196)

* Change `QuadraticProgramConverter.interpret` to convert `x` directly. 
* Change `OptimizationAlgorithm._interpret` to return a subclass of `OptimizationResult` by setting variables and status automatically.
* Remove deprecated methods `QuadraticProgramConverter.{encode,decode}`.
* Remove deprecated classes `QuadraticProgramToIsing` and `IsingToQuadraticProgram`.

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
Co-authored-by: Anton Dekusar <62334182+adekusar-drl@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stefan-woerner stefan-woerner stefan-woerner approved these changes

@adekusar-drl adekusar-drl adekusar-drl left review comments

@a-matsuo a-matsuo a-matsuo left review comments

@manoelmarques manoelmarques Awaiting requested review from manoelmarques

@pbark pbark Awaiting requested review from pbark

@woodsp-ibm woodsp-ibm Awaiting requested review from woodsp-ibm

Assignees
No one assigned
Labels
Changelog: API Change Include in the Changed section of the changelog Changelog: Deprecation Include in Deprecated section of changelog Optimization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@t-imamichi @adekusar-drl @a-matsuo @woodsp-ibm @stefan-woerner @manoelmarques