Skip to content

Adds translators between modeling libraries and quadratic program#122

Merged
manoelmarques merged 68 commits intoqiskit-community:mainfrom
t-imamichi:translator2
Jun 14, 2021
Merged

Adds translators between modeling libraries and quadratic program#122
manoelmarques merged 68 commits intoqiskit-community:mainfrom
t-imamichi:translator2

Conversation

@t-imamichi
Copy link
Copy Markdown
Collaborator

@t-imamichi t-imamichi commented May 10, 2021

Summary

Revised version of #116.

It introduces a model translators to convert a modeling libraries (e.g., docplex, gurobipy, etc.) to QuadraticProgram and vice versa.

It adds qiskit_optimization.translators directory and the following functions are implemented.

  • from_docplex_mp
  • to_docplex_mp
  • from_gurobipy
  • to_gurobipy

Because I plan to implement docplex.cp version of translators in the future, I use the name docplex_mp instead of just docplex.

Fixes #80

TODO

  • class design
  • naming of methods
  • unit tests
  • Gurobi translator
  • update unit tests that use QP.from_docplex or QP.to_docplex.
  • update tutorials that use QP.from_docplex or QP.to_docplex.

Details and comments

Deprecations

  • QuadraticProgram.from_docplex: will be replaced with from_docplex_mp.
  • QuadraticProgram.to_docplex: will be replaced with to_docplex_mp

Removal

Comment thread qiskit_optimization/translators/utils.py Outdated
Comment thread qiskit_optimization/translators/model_translator.py Outdated
Comment thread qiskit_optimization/translators/utils.py Outdated
Comment thread qiskit_optimization/problems/quadratic_program.py Outdated
Comment thread qiskit_optimization/translators/model_translator.py Outdated
Comment thread qiskit_optimization/problems/quadratic_program.py Outdated
Comment thread qiskit_optimization/translators/__init__.py Outdated
Comment thread qiskit_optimization/problems/quadratic_program.py Outdated
@t-imamichi
Copy link
Copy Markdown
Collaborator Author

t-imamichi commented May 27, 2021

I notice that we may be able to simplify QP.read_from_lp_file and QP.write_to_lp_file by making a translator. I will work on it.

@t-imamichi t-imamichi changed the title [WIP] Translation framework between modeling libraries and quadratic program Translation framework between modeling libraries and quadratic program Jun 7, 2021
@t-imamichi t-imamichi marked this pull request as ready for review June 7, 2021 10:28
@t-imamichi
Copy link
Copy Markdown
Collaborator Author

I have updated unit tests and tutorials so that everything uses the new from_docplex_mp and to_docplex_mp.

@woodsp-ibm
Copy link
Copy Markdown
Member

I created an Issue in Finance - and linked it as you can see here above - since the docplex based applications there should be updated to make them compatible with this change.

@t-imamichi
Copy link
Copy Markdown
Collaborator Author

I'm not sure why sphinx-spelling reports "October" is misspelling. But, I add it to pylintdict to pass CI, anyways.
https://github.com/Qiskit/qiskit-optimization/runs/2814027705

/home/runner/work/qiskit-optimization/qiskit-optimization/docs/release_notes.rst:64: Spell check: october: which were deprecated in Qiskit Aqua 0.8.0 release (October 2020)..

@t-imamichi t-imamichi added Changelog: New feature Include in the Added section of the changelog priority: high labels Jun 13, 2021
@manoelmarques
Copy link
Copy Markdown
Contributor

manoelmarques commented Jun 13, 2021

As for having some template for deprecation msgs that @woodsp-ibm mentioned. I had created a set of methods in Qiskit-Aqua at https://github.com/Qiskit/qiskit-aqua/blob/main/qiskit/aqua/deprecation.py.

I will create something similar to be used by the application projects.

@t-imamichi
Copy link
Copy Markdown
Collaborator Author

Thank you. I fixed the deprecation message following @woodsp-ibm's advice. If your template is merged before the review of this PR, I use it.

@manoelmarques
Copy link
Copy Markdown
Contributor

manoelmarques commented Jun 13, 2021

@t-imamichi If you intend to finish this PR before Monday, feel free to do it. I will be looking at this template on Monday and looking at all repo usages so that it applies to everything. It may not be ready before you finish this PR. If your PR is already merged, I will change the code to use the template methods.

@t-imamichi
Copy link
Copy Markdown
Collaborator Author

I have finished the revision of PR just now. I need to wait for review.

@woodsp-ibm
Copy link
Copy Markdown
Member

Looks good but one final thing. If you look in the combined deprecation output in CI there are 4 warnings being emitted related to to/from docplex. Maybe these are from unit tests but I can't tell. I think normally we would have stacklevel=2 on the warning so it refers to the caller - so it would be good to change the warnings to set that as it helps the end user too, who may have code they need to change.

If these warnings are from unit tests, normally there in the test we suppress and re-enable the warning around the code that we know will emit it so we don't end up seeing these anymore in CI. This is as ideally we should not be emitting deprecation warnings, by using Qiskit code, whether from other modules or tutorials, when we release. So avoiding warnings appearing from unit tests helps keep the log clean and easier to deal with the main code - now there are some warnings from 3rd party libraries, these are usually unavoidable until the library itself updates.

@t-imamichi
Copy link
Copy Markdown
Collaborator Author

t-imamichi commented Jun 14, 2021

OK. The deprecation message was due to the unit test test_docplex of from_docplex and to_docplex themselves. I removed them. 958726a

@woodsp-ibm
Copy link
Copy Markdown
Member

Ok, normally we continue to test deprecated function while it exists to make sure it continues to work while we support it, ahead of its removal, but then suppress the deprecation warning around the test code as we know calling it will emit a warning. Anyway in this case I guess the risk is small in having no test coverage since the methods just simply call across to code that is tested.

It would be good even so though on the warnings.warn calls to have stacklevel=2 set so the site of where its called from is shown in the messages, otherwise it shows the function itself which can make it hard to find/know where its being called from.

@t-imamichi
Copy link
Copy Markdown
Collaborator Author

t-imamichi commented Jun 14, 2021

I see. It is safer to keep test_docplex. I reverted the removal, added stacklevel=2 to warnings.warn, and suppressed the deprecation warnings in test_docplex test.

Copy link
Copy Markdown
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.

I think we are good to go now. @t-imamichi thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: New feature Include in the Added section of the changelog priority: high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

General conversion interface between QuadraticProgram and modeling libraries

6 participants