Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more config parameters to HiGHS python API + set up logging #606

Merged
merged 3 commits into from
Jul 15, 2023

Conversation

siwy
Copy link
Contributor

@siwy siwy commented Dec 6, 2022

Reference issue

#553 (sort of )

What does this implement?

Improves the usability of the HiGHS interface:

  • Pass more of pulp's parameters directly into HiGHS
  • Set-up logging (it's a bit awkward, I wasn't sure if there's a better way to do this)
  • Allow callers to specify more options via the options argument

Any other comments

N/A

@siwy
Copy link
Contributor Author

siwy commented Dec 12, 2022

Hi @pchtsp - no rush, but let me know if this is good enough to be merged

@siwy siwy changed the title Add more config parameters + try to set up logging Add more config parameters to HiGHS python API + try to set up logging Dec 13, 2022
@siwy
Copy link
Contributor Author

siwy commented Mar 10, 2023

Hi @pchtsp - pinging again, would it be possible to merge those changes?

@siwy siwy changed the title Add more config parameters to HiGHS python API + try to set up logging Add more config parameters to HiGHS python API + set up logging Mar 10, 2023
Copy link
Collaborator

@pchtsp pchtsp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I added just a minor comment on the arguments, so that it matches the other APIs structure (and also so it shows in the docs).

def __init__(
self,
mip=True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we prefer having the explicit enumeration of parameters, together with a description. This produces the documentation of the APIs in the docs. It's the only way to know in the docs which parameters are supported and which ones are not.
See

pulp/pulp/apis/cplex_api.py

Lines 296 to 316 in a017fdb

def __init__(
self,
mip=True,
msg=True,
timeLimit=None,
gapRel=None,
warmStart=False,
logPath=None,
epgap=None,
logfilename=None,
):
"""
:param bool mip: if False, assume LP even if integer variables
:param bool msg: if False, no log is shown
:param float timeLimit: maximum time for solver (in seconds)
:param float gapRel: relative gap tolerance for the solver to stop (in fraction)
:param bool warmStart: if True, the solver will use the current value of variables as a start
:param str logPath: path to the log file
:param float epgap: deprecated for gapRel
:param str logfilename: deprecated for logPath
"""

@siwy siwy force-pushed the maciej/add-highs-logging-and-options branch 2 times, most recently from b73e6d4 to eb7edf3 Compare March 24, 2023 10:26
@siwy
Copy link
Contributor Author

siwy commented Mar 24, 2023

@pchtsp - Thanks for the review, that makes a lot of sense! I've added the explicit parameters and docs back
(sorry for the delay, I had a small bout of flu)

@pchtsp
Copy link
Collaborator

pchtsp commented Apr 1, 2023

Hello again and sorry for the delay. In the meantime, we merged some other PRs for highs api. Can you fix the conflicts and re-merge? That way I can approve. thanks again!

@siwy siwy force-pushed the maciej/add-highs-logging-and-options branch from dfd66a7 to 665de2e Compare April 9, 2023 16:03
@siwy siwy force-pushed the maciej/add-highs-logging-and-options branch 2 times, most recently from 8ba2ef0 to 6f69ee2 Compare April 9, 2023 17:06
@siwy siwy force-pushed the maciej/add-highs-logging-and-options branch from 6f69ee2 to c6460a9 Compare April 9, 2023 17:09
@siwy
Copy link
Contributor Author

siwy commented Apr 9, 2023

@pchtsp - all done

@pchtsp pchtsp merged commit 6a8e463 into coin-or:master Jul 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants