-
-
Notifications
You must be signed in to change notification settings - Fork 389
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
Parsing objective sense in MPS file #656
base: master
Are you sure you want to change the base?
Conversation
Stephen J. Maher 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. |
1 similar comment
Stephen J. Maher 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. I'm still unsure if we should where we should check for None and where to assign the mode of the line. I've left some comments.
parameters['sense'] = const.LpMaximize if line[1] == 'MAX'\ | ||
else const.LpMinimize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't you missing a mode=CORE_FILE_OBJSENSE_MODE
here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in this case. This line is catching the case where you have the objective sense provided in the same line as the keyword OBJSENSE
, such as
OBJSENSE MAX
In this case we don't need to enter the OBJSENSE mode.
else: | ||
mode = CORE_FILE_OBJSENSE_MODE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why would we have the same mode regardless of the if clause. Maybe we should put the assignment before the len(line)>1
and take out the else
clause? Or maybe I don't understand the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following on from the previous comment, we have to enter the CORE_FILE_OBJSENSE_MODE
is the objective sense is not given in the same line as the OBJSENSE
keyword. This is catching the case
OBJSENSE
MAX
The check of len(line) > 1
differentiates between these two cases.
elif mode == CORE_FILE_OBJSENSE_MODE: | ||
parameters['sense'] = const.LpMaximize if line[0] == 'MAX'\ | ||
else const.LpMinimize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we test if parameters['sense']
is None? or if we provided an argument to the function (i.e., via the sense
argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could put in a check here for whether parameters['sense']
is None
. However, in the current code the check is not necessary since it is only possible to enter the CORE_FILE_OBJSENSE_MODE
mode if parameters['sense']
is None
.
We could put in an assert or throw an exception to protect against future code changes? Something like (after line 154)
if parameters['sense'] is not None
raise Exception("The supplied objective sense will be overwritten by the MPS file objective sense")
Fixes #655
This PR makes it possible to set the objective sense in an MPS file. Currently, the objective sense in the MPS file is ignored. The sense needs to be supplied to
fromMPS
as a parameter.This change does the following:
sense
parameter inLpProblem.fromMPS
now defaults toNone
.readMPS
methods checks for the headerOBJSENSE
in the MPS file. If the header is encounter there are two options:or
both options are handled in the
readMPS
method.LpMinimize
.LpProblem.fromMPS
, then this overrides the sense that is specified in the MPS file.