Add MPS and GZ support in translators
#583
Conversation
Pull Request Test Coverage Report for Build 8389542132Details
💛 - Coveralls |
dthuerck
requested review from
stefan-woerner,
woodsp-ibm,
t-imamichi and
Sofranac-Boro
as code owners
There was a problem hiding this comment.
A couple of quick comments. And it will need a release note for the new function and to note the deprecation see https://github.com/qiskit-community/qiskit-optimization/blob/main/CONTRIBUTING.md#release-notes for info on this, if you need it, which is done with a tool called reno.
| " _e f\n", | ||
| "End\n", | ||
| "\n" | ||
| "ename": "NameError", |
There was a problem hiding this comment.
It looks like perhaps just this cell was run - probably best to re-run the whole notebook locally for whats checked in. It will get re-run too when docs are built but it should be checked in a state that would match what CI produces.
There was a problem hiding this comment.
Thanks - re-ran and checked it.
| @@ -917,45 +916,47 @@ def export_as_lp_string(self) -> str: | |||
| Returns: | |||
| A string representing the quadratic program. | |||
| """ | |||
| warn( | |||
There was a problem hiding this comment.
There are some deprecation decorators in Qiskit that we try and use now. They also emit the deprecation into the docstring so the notices come out in the published docs. You can most likely find usage examples in Qiskit - this is the file with the decorators https://github.com/Qiskit/qiskit/blob/main/qiskit/utils/deprecation.py
There was a problem hiding this comment.
Thanks again for the reference - used the appropriate decorators in the last commit.
|
@woodsp-ibm Thanks for the hints! Should be all done by now, release note is there. |
There was a problem hiding this comment.
One other thing. Its now 2024 and many of these files still have 2023 dates. This passes on CI presently but, from past experience, the files, when merged in to main will end up have last updated dates of 2024. This PR will not fail, but any subsequent ones, since the checks are run against all files, will fail on these despite it might not even touch them. To avoid this, if I could ask you to update the dates for 2024 in all the files with a copyright that will avoid us having to deal with it down the line. Its a bit of a pain I know but having a PR cross a year boundary has been more of an exception.
| @@ -0,0 +1,12 @@ | |||
| --- | |||
| features: | |||
There was a problem hiding this comment.
This file should be in releasenotes/notes - if you ran the reno command that is where it should have created it. We only move the notes into a folder under there for the version at the point its released.
Bear in mind too this should be describing things user perspective, and since we expect people to import these from translators the extra "unit"/module is arguably an implementation detail. And as far as I can see there are functions to read and write .mps and .lp files and additionally for the read it supports gzipped files i.e. .mps.gz and .lp.gz (I did not see any write capatibility for gz unless I missed it). It might be list the functions here so someone can click and go to the API ref from the release note here. Often we would have a small sample code for new function but I think this is simple enough that its not needed, right.
| @@ -44,7 +45,7 @@ def test_cplex_optimizer(self, config): | |||
| # load optimization problem | |||
| problem = QuadraticProgram() | |||
| lp_file = self.get_resource_path(filename, "algorithms/resources") | |||
| problem.read_from_lp_file(lp_file) | |||
| problem = read_from_lp_file(lp_file) | |||
There was a problem hiding this comment.
Ideally we continue to test the deprecated function to ensure that it still works while its supported before we remove it. In this case maybe this is ok knowing that the deprecated code just simply forwards the call to the new function - hopefully its hard for that to fail before its removed.
| def test_read_from_lp_gz_file(self): | ||
| """test read compressed lp file""" | ||
| try: | ||
| with self.assertRaises(IOError): |
There was a problem hiding this comment.
This test is bundling together a number of tests. If it fails the first here the rest of the tests won't be run and that might provide insight into the failure - does depend on what the tests/code are and it may not in this case. Usually we try to do with sub-test, or parameterize a testt with ddt, eg since all the failure tests are the same but the data that part of could be done that way. I see the helper above is a ton of asserts - and that may be overkill for that though again its going to stop the test on the first failure of course. Something to think about anyway.
| @@ -34,6 +34,14 @@ | |||
| """ | |||
|
|
|||
| from .docplex_mp import from_docplex_mp, to_docplex_mp | |||
| from .file_io import ( | |||
There was a problem hiding this comment.
We should include these in the documentation as they are public methods we now expect a user to use. You can see the pre-existing list above in the docstring - maybe it makes sense to have another section that lists these - or perhaps the export ones go under Translators there and the others under "File loading and saving" ?
You can see what the current page looks like from here https://qiskit-community.github.io/qiskit-optimization/apidocs/qiskit_optimization.translators.html. To build docs locally runmake html in the project root, or make clean html which will wipe away whats there which sometimes is needed if the incremental build does not come out as expected - I find this more if I change the structure. One or more docstrings changed is pretty much ok. html will be in the _build folder that is created under docs
If you want to see how the docs come out in the build, if you go to build Details and Summary you can scroll down and see artifacts that the build creates. documentation is the docs built without running the tutorials as a quicker check. tutorials_3.x are jobs under that version of Python that also build the docs but run the notebooks too.
|
Thanks for the extensive comments, @woodsp-ibm ! I added test cases for the deprecated functions and updated the docs accordingly. |
|
Daniel Thuerck seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
✅ I have added the tests to cover my changes.
✅ I have updated the documentation accordingly.
✅ I have read the CONTRIBUTING document.
Summary
See issue #582: This PR succeeds #581, having moved lp/mps/gz IO functions into
translators.Details and comments
lpfunctions inQuadraticProgramhave been marked as deprecated. Tests and Tutorials have been updated to use thefile_io.