Add SAS API interface #729
Conversation
|
|
There was a problem hiding this comment.
Thanks for the contribution. Please find some comments. I haven't checked everything but this should already be a good start.
I've also seen that the two python packages are available through pypi.
- Can you add the solver to the github CI? example: ?
pulp/.github/workflows/pythonpackage.yml
Line 32 in 6e18ecb
- Can you add the solver to the testing suite? example: ?
Line 1576 in 6e18ecb
pulp/apis/sas_api.py
Outdated
| msg=msg, | ||
| ) | ||
| self._sas = saspy.SASsession() | ||
| self.warmStart=warmStart |
There was a problem hiding this comment.
warmstart can be passed to LpSolver and then becomes available inside self.optionsDict. See how it's used in other solvers. No need to assign it to the object.
| self.warmStart=warmStart | ||
| # Only named options are allowed in SAS solvers. | ||
| self.solverOptions = {key:value for key, value in solverParams.items()} | ||
| def available(self): |
There was a problem hiding this comment.
I believe you need to run the black linter on the code.
https://coin-or.github.io/pulp/develop/contribute.html#applying-the-black-linter-formatter
pulp/apis/sas_api.py
Outdated
|
|
||
| return (f"{name}-pulp.{n}" for n in args) | ||
|
|
||
| def _delete_tmp_files(self, *args): |
There was a problem hiding this comment.
there already exists a delete_tmp_files method that does almost exactly this. No need to create a new one. The same goes for _create_tmp_files
pulp/apis/sas_api.py
Outdated
| decompsubprob_str = self._create_statement_str("decompsubprob") | ||
| rootnode_str = self._create_statement_str("rootnode") | ||
|
|
||
| if lp.isMIP(): |
There was a problem hiding this comment.
simpler:
if lp.isMIP() and not self.mip:
warnings.warn("SAS94 will solve the relaxed problem of the MILP instance.")
pulp/apis/sas_api.py
Outdated
| primalin=tmpMst, | ||
| postfix=postfix, | ||
| ) | ||
| self.solverOptions["primalin"] = f"primalin{postfix}" |
There was a problem hiding this comment.
it's not a good practice to edit the solverOptions attribute. You should avoid editing during solve. It should be stateless.
pulp/apis/sas_api.py
Outdated
| self._delete_tmp_files(tmpMps, tmpMst) | ||
|
|
||
| # Store log and ODS output | ||
| self._log = res["LOG"] |
There was a problem hiding this comment.
I do not see this property being defined anywhere: _log. Why do we need to store it inside self?
| self._log = res["LOG"] | ||
| if self.msg: | ||
| print(self._log) | ||
| self._macro = dict( |
There was a problem hiding this comment.
same goes for _macro. is it needed after the solving? why?
There was a problem hiding this comment.
_macro is used to store some information returned by SAS solvers. It has information like primal and dual infeasibility, and SAS solution status. It might be interesting to SAS users.
pulp/apis/sas_api.py
Outdated
| raise PulpSolverError("PuLP: Error ({err_name}) \ | ||
| while trying to solve the instance: {name}".format( | ||
| err_name=self._macro.get("STATUS", "ERROR"), name=lp.name)) | ||
| status = self._read_solution(lp, primal_out, dual_out) |
There was a problem hiding this comment.
what if there are no values available? should we read the soluton still?
There was a problem hiding this comment.
If no values available, the solution will be NaN.
pulp/apis/sas_api.py
Outdated
| except FileNotFoundError: | ||
| pass | ||
|
|
||
| def _write_sol(self, filename, vs): |
There was a problem hiding this comment.
there are many methods that are common between the two solvers. Could we just inherit one from the other and get all those methods?
| raise PulpSolverError("SASCAS : Objective sense should be min or max.") | ||
|
|
||
| status = None | ||
| with redirect_stdout(SASLogWriter(self.msg)) as self._log_writer: |
There was a problem hiding this comment.
this part has many indentations and ifs. Can it be refactored so that it's easier to read? for example breaking it into other methods. re-using the code with different arguments, etc.
|
Hello, I solved the conflicts with master. Now the tests are failing because you need to run the black linter as I previously said: https://coin-or.github.io/pulp/develop/contribute.html#applying-the-black-linter-formatter |
There was a problem hiding this comment.
Thanks.
I've left more comments again. You need to get your code to run in the pulp CI, so fix the black linter first.
If someone else from SAS can help you reviewing the python code before asking for a review here, that would be awesome. I do not have too much time to give the amount of feedback/ improvements that is needed for this PR.
pulp/apis/sas_api.py
Outdated
|
|
||
| def _create_statement_str(self, statement): | ||
| """Helper function to create the strings for the statements of the proc OPTLP/OPTMILP code.""" | ||
| stmt = self.solverOptions.pop(statement, None) |
There was a problem hiding this comment.
please do not edit this property, it should be static. There are many places where it's edited in the code.
pulp/apis/sas_api.py
Outdated
| except: | ||
| return False | ||
|
|
||
| def toDict(self): |
There was a problem hiding this comment.
(1) you have this function duplicated.
(2) you should not just copy the inherit method, you should call the LpSolver_CMD.toDict method and then add the functionality that you need.
(3) What is cas object and how do you expect to serialize it?
pulp/apis/sas_api.py
Outdated
| return True | ||
|
|
||
| def sasAvailable(self): | ||
| if self.cas == None: |
Add API interface of SAS solvers.